Skip to content

Ethernet library: add missing functions #160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: arduino
Choose a base branch
from

Conversation

leonardocavagnis
Copy link
Member

@leonardocavagnis leonardocavagnis commented Jul 31, 2025

  • Add missing functions
  • Move Ethernet files in a separated library (Ethernet)
  • Add examples

@leonardocavagnis leonardocavagnis marked this pull request as draft July 31, 2025 16:33
@leonardocavagnis leonardocavagnis marked this pull request as ready for review August 6, 2025 14:29
Comment on lines +52 to +62
void MACAddress(uint8_t *mac_address);
IPAddress localIP();
IPAddress subnetMask();
IPAddress gatewayIP();
IPAddress dnsServerIP();

void setMACAddress(const uint8_t *mac_address);
void setLocalIP(const IPAddress local_ip);
void setSubnetMask(const IPAddress subnet);
void setGatewayIP(const IPAddress gateway);
void setDnsServerIP(const IPAddress dns_server);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid overloading these methods

}
public:
NetworkInterface() {}
~NetworkInterface() {}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make this virtual

Suggested change
~NetworkInterface() {}
virtual ~NetworkInterface() = default;

Comment on lines +28 to +35
static void event_handler(struct net_mgmt_event_callback *cb,
uint64_t mgmt_event,
struct net_if *iface);

static void option_handler(struct net_dhcpv4_option_callback *cb,
size_t length,
enum net_dhcpv4_msg_type msg_type,
struct net_if *iface);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make these methods into static functions in cpp file only? this way they are going to be hidden from the class definition

Copy link

@andreagilardoni andreagilardoni Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the curly braces consistent in style?either on new line or at the end of the line

Comment on lines +47 to +50
void init(uint8_t sspin = 10);

int disconnect(void);
void end(void);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do not need these

virtual ~EthernetClass() {}

int begin(uint8_t *mac = nullptr, unsigned long timeout = 60000, unsigned long responseTimeout = 4000);
int maintain();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method not needed

bool disconnect() {
return (net_if_down(netif) == 0);
}
int begin(bool blocking = true, uint32_t additional_event_mask = 0);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this virtual

Comment on lines +30 to +46
int begin(uint8_t *mac, IPAddress ip);
int begin(uint8_t *mac, IPAddress ip, IPAddress dns);
int begin(uint8_t *mac, IPAddress ip, IPAddress dns, IPAddress gateway);
int begin(uint8_t *mac, IPAddress ip, IPAddress dns, IPAddress gateway, IPAddress subnet, unsigned long timeout = 60000, unsigned long responseTimeout = 4000);

int begin(IPAddress ip) {
return begin(nullptr, ip);
}
int begin(IPAddress ip, IPAddress dns) {
return begin(nullptr, ip, dns);
}
int begin(IPAddress ip, IPAddress dns, IPAddress gateway) {
return begin(nullptr, ip, dns, gateway);
}
int begin(IPAddress ip, IPAddress dns, IPAddress gateway, IPAddress subnet) {
return begin(nullptr, ip, dns, gateway, subnet);
}
Copy link

@andreagilardoni andreagilardoni Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to add fewer begins without the matrioska pattern to help the compiler to optimize the code. I would define only these two: @pennam what do you think about that?

int begin() override;
int begin(IPAddress ip, IPAddress dns=INADDR_NONE, IPAddress gateway=INADDR_NONE, IPAddress subnet=INADDR_NONE);

@@ -66,9 +66,12 @@ EXPORT_SYMBOL(usb_disable);

EXPORT_SYMBOL(z_log_msg_runtime_vcreate);

FORCE_EXPORT_SYM(log_dynamic_sketch)
Copy link

@andreagilardoni andreagilardoni Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FORCE_EXPORT_SYM(log_dynamic_sketch)
#if defined(CONFIG_LOG)
FORCE_EXPORT_SYM(log_dynamic_sketch)
#endif // CONFIG_LOG

@@ -1,4 +1,211 @@
#include "SocketHelpers.h"

#include <zephyr/logging/log.h>
LOG_MODULE_DECLARE(sketch, LOG_LEVEL_NONE);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about using a log module named "sketch_networking" or something that is less generic than sketch?

Copy link

@andreagilardoni andreagilardoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would personally add a subset of examples that we are going to test in depth with a larger set of boards i.e. WebServer, WebClientRepeating, WebClientRepeatingSSL, UDPNtpClient. What do you think @pennam?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants