Compatibility with lwIP and ESP-IDF #13

snej · 2018-07-02T17:58:49Z

lwIP = Lightweight IP library
http://savannah.nongnu.org/projects/lwip/

ESP-IDF = iOT Development Framework for Espressif's ESP32 chip
https://github.com/espressif/esp-idf

New preprocessor flags for configuration:

NYOCI_LWIP -- define if compiling for lwIP. Will be automatically
defined if ESP_PLATFORM is defined (i.e. building for ESP-IDF.)
NYOCI_CAN_POLL -- Indicates the poll system call is available and
should be used instead of select. Defaults to 1 unless building
for lwIP; explicitly define it as 0 to force use of select.
NYOCI_CAN_SENDMSG -- Indicates the sendmsg system call is
available and should be used instead of send. Defaults to 1 unless
building for lwIP; explicitly define it as 0 to force use of send.
Note: send does not support setting the “from” address of the
packet.

Fixes #8

(I also added a one-line commit that fixes a recent regression on the master branch
that I discovered when rebasing just now.)

lwIP = Lightweight IP library <http://savannah.nongnu.org/projects/lwip/> ESP-IDF = iOT Development Framework for Espressif's ESP32 chip <https://github.com/espressif/esp-idf> New preprocessor flags for configuration: * NYOCI_LWIP -- define if compiling for lwIP. Will be automatically defined if ESP_PLATFORM is defined (i.e. building for ESP-IDF.) * NYOCI_CAN_POLL -- Indicates the `poll` system call is available and should be used instead of `select`. Defaults to 1 unless building for lwIP; explicitly define it as 0 to force use of `select`. * NYOCI_CAN_SENDMSG -- Indicates the `sendmsg` system call is available and should be used instead of `send`. Defaults to 1 unless building for lwIP; explicitly define it as 0 to force use of `send`. Note: `send` does not support setting the “from” address of the packet.

`==` has higher precedence than `&`, so the expression wasn’t doing what was intended.

darconeous · 2018-07-02T18:16:12Z

I think a better approach would be to create a net platform rather than modifying the posix platform in-place, even if the new platform is based on posix.

Maybe call it plat-net/lwip-sockets?

darconeous · 2018-07-03T18:18:55Z

src/libnyoci/nyoci-outbound.c

@@ -320,7 +320,7 @@ nyoci_outbound_add_options_up_to_key_(

 #if NYOCI_CONF_TRANS_ENABLE_OBSERVING
 	if ( (self->current_transaction != NULL)
-	  && (self->current_transaction->flags & NYOCI_TRANSACTION_OBSERVE == NYOCI_TRANSACTION_OBSERVE)
+	  && ((self->current_transaction->flags & NYOCI_TRANSACTION_OBSERVE) == NYOCI_TRANSACTION_OBSERVE)


Good catch! Mind pulling this commit out into it's own pull request?

I can't claim credit -- GCC found it and warned me (but I can't remember which warning flag it was.)
I'll submit a separate PR when I have a chance.

snej · 2018-07-06T16:39:23Z

I think a better approach would be to create a net platform rather than modifying the posix platform in-place, even if the new platform is based on posix.

But that would mean duplicating ~1200 lines of code, which I couldn't bring myself to do (I'm pretty strict about DRY.)

Consider also that most of the changes aren't specific to lwIP; they'll add support for any other POSIX-like platform that's missing poll and/or sendmsg.

But it is of course your call. I may just keep my fork, err, forked. In that case, mind if I submit a smaller PR for just the ESP32-specific changes (e.g. to logging?)

darconeous · 2018-07-07T07:06:19Z

I'll take another look over the changes. If there are other POSIX platforms that your changes help make LibNyoci build on (Solaris? QNX? Minix?), then I'd be inclined to accept it. QNX in particular might benefit from it.

But I may end up just going ahead and implementing support for native LWIP.

Travis is failing here with these changes:

../../../../src/plat-net/posix/nyoci-plat-net.c:922:4: error: field designator 'sin6_len' does not refer to any field in type 'struct sockaddr_in6'
                .sin6_len = sizeof(struct sockaddr_in6),
                 ^

But let me review this stuff before you spend any significant amount of time trying to address that.

darconeous · 2018-07-07T07:10:55Z

src/libnyoci/assert-macros.h

@@ -79,6 +80,12 @@
 				PSTR(__FILE__ ":%d: "fmt"\n"), \
 				__LINE__, \
 				__VA_ARGS__)
+ #elif ESP_PLATFORM


I like all of the changes in this file, including the errno stuff. May be worth spinning out.

darconeous

I'm still reviewing, but I like several of your changes so far. There are a few minor style issues (some spaces snuck in where tabs should be, etc), but I won't nit-pick those until I'm sure I grok the whole picture—which I will try to do in the morning.

darconeous · 2018-07-07T07:17:43Z

src/libnyoci/nyoci-logging.h

@@ -34,40 +34,46 @@

 #if !VERBOSE_DEBUG

-#define CSTR(x)     (x)
+	#define CSTR(x)     (x)


I'd rather significant changes in the whitespace not be combined with material additions or changes to a file, unless it is unavoidable. It makes the diffs harder to read.

That aside, I'm having trouble getting my brain to not be weirded out by the use of full indents, though. That's largely why I used single-space indents in assert-macros.h: indented preprocessor macros look weird to me after 20+ years of not indenting them at all. I found the single-space indentation improved readability for assert-macros.h without looking too weird to my eyes, but maybe I'm just being stubborn and tabs are really the way to go.

darconeous · 2018-07-07T07:32:16Z

src/plat-net/posix/nyoci-plat-net.c

@@ -230,7 +287,7 @@ nyoci_plat_init(nyoci_t self) {
 #if NYOCI_PLAT_NET_POSIX_FAMILY == AF_INET6
 	if (self->plat.mcfd_v6 == -1) {
 		self->plat.mcfd_v6 = socket(AF_INET6, SOCK_DGRAM, 0);
-		check(self->plat.mcfd_v6 >= 0);
+		check_errno(self->plat.mcfd_v6 >= 0);


+1 for all of these sorts of changes.

darconeous · 2018-07-07T07:36:21Z

src/plat-net/posix/nyoci-plat-net.c

 		ret = sendmsg(fd, &msg, flags);

 		check(ret > 0);
 		check_string(ret >= 0, strerror(errno));
+#else // !NYOCI_CAN_SENDMSG
+		abort(); //TODO


It's a little hard to tell what is going on here from looking at the diff on GitHub. Why is this abort() here? Would an #error be more suitable?

darconeous · 2018-07-07T07:39:57Z

src/plat-net/posix/nyoci-plat-net.c

+	}
+	ret = nyoci_plat_update_fdsets(self, &reads, &writes, &errors, &fdCount, &cms);
+	if (ret != NYOCI_STATUS_OK)
+		goto bail;


I know there is a lot of old code in LibNyoci that doesn't use braces for single-line if() statements, but ever since the goto fail bug I require the use of curly braces for all if() statements in all new code, even if copy-pasted.

darconeous · 2018-07-26T18:31:53Z

Good news: It looks like I'll be adding real native support for LWIP at some point between now and the end of November. I'll also be adding support for bare-bones OpenThread.

darconeous · 2018-07-26T18:33:40Z

You've got some good changes in this PR, so I'm going to keep this open for now.

Mostly so that an outgoing packet can be logged by coap_dump_header.

strtol returns an long, not a pointer.

It wasn’t at all obvious how to specify the CoAP URL, without browsing the source code...

Added FETCH/PATCH and status codes from RFC 8132. Changed Internet-Draft references to the RFCs they became.

snej added 2 commits July 2, 2018 10:45

Fixed a logic error due to missing parens

b431d10

`==` has higher precedence than `&`, so the expression wasn’t doing what was intended.

darconeous reviewed Jul 3, 2018

View reviewed changes

Xcode project: define DEBUG=1, not just DEBUG

e109fca

darconeous reviewed Jul 7, 2018

View reviewed changes

snej added 7 commits August 20, 2018 17:32

Xcode: Lower MACOSX_DEPLOYMENT_TARGET to 10.10

10d16d6

Added some missing cases to xxx_to_cstr fns

d4f5bb7

Added nyoci_outbound_get_packet[_length]()

da3f2f9

Mostly so that an outgoing packet can be logged by coap_dump_header.

Fixed a compiler warning in nyocictl

380864c

strtol returns an long, not a pointer.

nyocictl: Added env var info to usage/help output

37319a5

It wasn’t at all obvious how to specify the CoAP URL, without browsing the source code...

Updated CoAP constants in coap.h

87451c0

Added FETCH/PATCH and status codes from RFC 8132. Changed Internet-Draft references to the RFCs they became.

Merge branch 'master' into feature/lwIP

c5526dd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility with lwIP and ESP-IDF #13

Compatibility with lwIP and ESP-IDF #13

snej commented Jul 2, 2018

darconeous commented Jul 2, 2018

darconeous Jul 3, 2018

snej Jul 6, 2018

snej commented Jul 6, 2018

darconeous commented Jul 7, 2018 •

edited

Loading

darconeous Jul 7, 2018

darconeous left a comment

darconeous Jul 7, 2018

darconeous Jul 7, 2018

darconeous Jul 7, 2018

darconeous Jul 7, 2018

darconeous commented Jul 26, 2018

darconeous commented Jul 26, 2018

Compatibility with lwIP and ESP-IDF #13

Are you sure you want to change the base?

Compatibility with lwIP and ESP-IDF #13

Conversation

snej commented Jul 2, 2018

darconeous commented Jul 2, 2018

darconeous Jul 3, 2018

Choose a reason for hiding this comment

snej Jul 6, 2018

Choose a reason for hiding this comment

snej commented Jul 6, 2018

darconeous commented Jul 7, 2018 • edited Loading

darconeous Jul 7, 2018

Choose a reason for hiding this comment

darconeous left a comment

Choose a reason for hiding this comment

darconeous Jul 7, 2018

Choose a reason for hiding this comment

darconeous Jul 7, 2018

Choose a reason for hiding this comment

darconeous Jul 7, 2018

Choose a reason for hiding this comment

darconeous Jul 7, 2018

Choose a reason for hiding this comment

darconeous commented Jul 26, 2018

darconeous commented Jul 26, 2018

darconeous commented Jul 7, 2018 •

edited

Loading