Skip to content
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

Small compatibility improvements for esp32 embedded platforms #741

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fedepell
Copy link

@fedepell fedepell commented Apr 2, 2024

This PR adds a few small compatibility improvements for embedded platforms, specifically for the esp32 family using ESP-IDF, which are based on lwip. With these small modifications and by providing an external nanosleep (no need to pollute libmodbus as one can provide it externally in the project) then the library can be used in TCP mode in ESP-IDF.

The changes are not enough for using it with serial ports, I may send another PR for that (it will be a little more complicated as there we need to change the init part using the UART library for that system).

Would you be interested to have that in case in the codebase? (of course with a separate #define to replace the modbus_rtu_connect when one chooses that specific platform, a bit like you already have WIN32/Posix differentiation)

Some environments, for example lwip, don't have it, so let's make
it configurable so we can also build and work on those platforms
by simplying using project configuration.
Check via usual autoconf and when not present print just the error
code instead of the readable string. Useful for compact embedded
stacks where this is not present.
Copy link

cla-bot bot commented Apr 2, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient...

@fedepell fedepell mentioned this pull request May 20, 2024
@@ -41,7 +41,9 @@
#endif

# include <netinet/in.h>
#ifdef HAVE_NETINET_IP_H
Copy link
Owner

Choose a reason for hiding this comment

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

ip.h provides IPTOS_LOWDELAYso how this setting is available on ESP32?

Copy link
Author

Choose a reason for hiding this comment

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

in lwip/sockets.h which comes from netdb.h (above)

@stephane
Copy link
Owner

Partial merges: 087667a and 10e33ec

@fedepell
Copy link
Author

Partial merges: 087667a and 10e33ec

Would have been nice/correct to have an attribution then, especially after asking to sign a CLA ;) (ie. with Co-authored-by)

@stephane
Copy link
Owner

@fedepell you're right, I don't use Co-authored-by but most of my commits keep the original author (rebase, etc) or I add a ref in commit. It's an oversight I'll add your name to the changelog.

@fedepell
Copy link
Author

@fedepell you're right, I don't use Co-authored-by but most of my commits keep the original author (rebase, etc) or I add a ref in commit. It's an oversight I'll add your name to the changelog.

Thanks and no worries please :) Hope we can (with the appropriate modifications and so on) to put the ESP support in, it works like a charm and is much better than the library provided in the SDK :)

@diplfranzhoepfinger
Copy link
Contributor

and i also overlooked this.

@@ -401,7 +403,11 @@ static int _modbus_tcp_pi_connect(modbus_t *ctx)
rc = getaddrinfo(ctx_tcp_pi->node, ctx_tcp_pi->service, &ai_hints, &ai_list);
if (rc != 0) {
if (ctx->debug) {
#ifdef HAVE_GAI_STRERROR
fprintf(stderr, "Error returned by getaddrinfo: %s\n", gai_strerror(rc));
Copy link
Contributor

Choose a reason for hiding this comment

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

why use gai_strerror and not strerror?

Copy link
Author

Choose a reason for hiding this comment

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

That's a good question ;) But my MR was only to make it conditional (since in ESP-IDF libs there isn't) not to really change the function used. But in case @stephane could consider that! (and then my change would not be needed indeed)

@diplfranzhoepfinger
Copy link
Contributor

The changes are not enough for using it with serial ports, I may send another PR for that (it will be a little more complicated as there we need to change the init part using the UART library for that system).

Hi,

my sample also runs at serial Port:
see: #766

@fedepell
Copy link
Author

The changes are not enough for using it with serial ports, I may send another PR for that (it will be a little more complicated as there we need to change the init part using the UART library for that system).

Hi,

my sample also runs at serial Port: see: #766

yeah as well as #745 :)

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.

3 participants