WIP: Rewrite sntp in Lua with only a little C

[nwf]

trafficstars

• [x] This PR is for the dev branch rather than for master.
• [x] This PR is compliant with the other contributing guidelines as well (if not, please describe why).
• [x] I have thoroughly tested my contribution.
• [x] The code changes are reflected in the documentation at docs/*.

This PR contains a new sntp module with almost all the state machine done in Lua and only the packet processing and fiddly large-integer math done in C. It is intended to supplant #2700 and the in-C in-progress rewrite I mentioned therein.

At present, it is exceptionally naive about time itself, even if its state machine endeavors to be more robustly implemented; @jmattsson's existing C displays a much more thorough understanding of time keeping than I possess. It seems likely that we will need, and I am not opposed to adding, additional userdata structures for persistent state in the sntppkt C half.

[jmattsson]

I'll try to have a look late this week / next week!

[TerryE]

Nathaniel, If you don't mind, I'll give a general review rather than one anchored to PR lines, Though I will add a few specific anchored comments.

• I find the overall concept of using hybrid modules an extremely interesting one and one that is really built around utilising LFS. This allows us to keep the C part minimalist and really private to the public Lua module which keeps the bulk of the login in a form that is accessible and extensible to the application programmer. With LFS this can incur to RAM penalty and also allow the developer to move decisions about whether SNTP is needed to LFS image load. I think that this is well worth exploring.

• Is it worth making the C module sntppkt truly private (or not easily accessible from general Lua) because if it is only used by the "trusted" SNTP module then we can slim down on some of the validation and error checking. One way of doing this would not to publicly document this C helper and also give it a non-compliant Lua variable name, e.g. sntp-helper,c. The SNTP constructor could still look up this up using the ROM['sntp-helper,c'] access path and stash this in a shared upval, and also throw an explicit error "You must include the SNTP_HELPER module" in your build to use SNTP" if this is nil.

• BTW, I strongly recommend recording all upvals used in each Lua function in its declaration e.g. with -- upvals: x, y, z as I do in my code, especially when said upvals are used RW within the routine and can therefore create side-effects. You can find these by mining the -l listing.

• Is the SNTP object intended to be a singleton or not? (I see that it is currently written as mutli-instance -- that is the application can create multiple SNTP servers). The reason that I ask this is that if it makes more sense to make it a singleton then the locals used as upvals / internal functions can be hoisted one level rather than including them inside new().

[nwf]

Nathaniel, If you don't mind, I'll give a general review rather than one anchored to PR lines, Though I will add a few specific anchored comments.

Any and all commentary is more than welcome. :)

• I find the overall concept of using hybrid modules an extremely interesting one and one that is really built around utilising LFS. This allows us to keep the C part minimalist and really private to the public Lua module which keeps the bulk of the login in a form that is accessible and extensible to the application programmer. With LFS this can incur to RAM penalty and also allow the developer to move decisions about whether SNTP is needed to LFS image load. I think that this is well worth exploring.

I confess the intent was not so much to take advantage of LFS as it was to get rid of some hairy C, but yes, LFS certainly makes this kind of design much more attractive. LFS really does change the programming landscape for the project.

• Is it worth making the C module sntppkt truly private (or not easily accessible from general Lua) because if it is only used by the "trusted" SNTP module then we can slim down on some of the validation and error checking. One way of doing this would not to publicly document this C helper and also give it a non-compliant Lua variable name, e.g. sntp-helper,c. The SNTP constructor could still look up this up using the ROM['sntp-helper,c'] access path and stash this in a shared upval, and also throw an explicit error "You must include the SNTP_HELPER module" in your build to use SNTP" if this is nil.

Oh, that's an intriguing idea. The sntppkt module is, at the moment, stateless, so there's presently no harm in exposing it, but I'm not planning on documenting it (heavily). I'm glad to know that private-ish/name-mangled is possible; that might well be the right answer here.

• BTW, I strongly recommend recording all upvals used in each Lua function in its declaration e.g. with -- upvals: x, y, z as I do in my code, especially when said upvals are used RW within the routine and can therefore create side-effects. You can find these by mining the -l listing.

Good plan; will do.

• Is the SNTP object intended to be a singleton or not? (I see that it is currently written as mutli-instance -- that is the application can create multiple SNTP servers). The reason that I ask this is that if it makes more sense to make it a singleton then the locals used as upvals / internal functions can be hoisted one level rather than including them inside new().

I think it is slightly simpler to have the application construct a new one and discard the old one if servers change, so I had not intended it to be a singleton. That said, maybe there's not that much gain in simplicity and nobody's ever going to want more than one sntp client instance anyway. It can go back to being a singleton like the existing one if there's desire.

[HHHartmann]

• Is it worth making the C module sntppkt truly private (or not easily accessible from general Lua) because if it is only used by the "trusted" SNTP module then we can slim down on some of the validation and error checking. ...

If we want to allow the Lua part to be extensible by the application programmer, we should not slim down on error checking too much. Also there is no guarantee that the user packs the matching version of the Lua module. So this should be able fail gracefully with a meaningful error message and not just "not work".

[nwf]

The intent was to expose callbacks from the Lua layer much as is done now but to limit the direct extensibility of sntp.lua itself, again, just as we currently discourage modification of (or at least, require acknowlegement of being on one's own when modifying) sntp.c.

[nwf]

I've spent a while more hacking on this and gotten it passing some basic functionality tests. It would be good if someone (esp. @jmattsson) could review it, but I think I continue to like the feel of "hard bits in C, easy bits in Lua" development rather than "self-contained, complete C modules".

[marcelstoer]

I don't feel I can add anything meaningful to the discussion here so maybe I better keep my mouth shut but I'm still tempted...sorry.

I continue to like the feel of "hard bits in C, easy bits in Lua" development rather than "self-contained, complete C modules".

Is that the perspective of the firmware/module maintainer or that of the module user? As an end user having a self-contained C module available feels more convenient to me personally (unless one uses LFS maybe). However, I accept that there are different kinds of "end-users". Terry stated

...private to the public Lua module which keeps the bulk of the login in a form that is accessible and extensible to the application programmer

and that's certainly a valid point. How many would want/need to extend that module, though?

[nwf]

From the LFS-capable end-user perspective, the two are interchangeable. Without LFS, it's hard to imagine NodeMCU as anything but a toy, and so I am not particularly worried about the heap bloat from the "easy bits in Lua" code.

I'm sorry to be harsh, but the quality of existing C modules suggests that very few developers, and I exclude myself, are capable of producing production-quality code.

[pjsg]

I think the this project suffers from only having a few significant contributors and there is a lot of legacy code from the past. I'd like to get the sntp module cleaned up, but it does have quite a lot of functionality today, some of which I added to make the module relatively foolproof (i.e. call it once at startup and then it does the right thing).

[nwf]

I will make the default success callback in this one DTRT, and will add a simple "just go" function to the module.

[nwf]

OK, I've done a lot more polishing on this and I believe I'm ready to propose that it land.

In terms of longer future, I suggest that we leave the existing sntp C module around unaltered for the next master drop and only mark it deprecated in documentation, the drop after that can add the deprecation warning to source, and the drop after that can remove the module. This should give us ample time to suss out any bugs with the fallback still around. Does that seem fair?

[TerryE]

@nwf, out of interest, if there any specific reason for us deciding not to use the SDK's built-in SNTP functions (SDK/include/sntp.h) and instead doing our own implementation?

BTW, the SNTP interface is documented in the non-OS SDK API reference and is (I believe) a backport of the ESP32 RTOS equivalent.

[nwf]

[Previous incorrect comment deleted]

It looks like I removed the LwIP sntp.c in c972d86ea9f86794c0d43250f4bc99c3814e0e4c since we had the existing one, which was more aware of the native hardware RTC. And I've replicated that RTC awareness here. We could try adding Lua callbacks to the LwIP sntp?

[TerryE]

It looks like I removed the LwIP sntp.c in c972d86 since we had the existing one, which was more aware of the native hardware RTC. And I've replicated that RTC awareness here. We could try adding Lua callbacks to the LwIP sntp?

We might have a perfectly good reason, but it was still worth asking the Q, I think. I think that Johny's original SMTP module predated Espressif's addition of its own.

[nwf]

Thanks for useful feedback. Here's a revised commit.

[HHHartmann]

Sorry, trying to trigger a CI build

[nwf]

CI builds are great, but could you please use a new PR that won't send me email? :)

[HHHartmann]

Sure, sorry to annoy you, I was just on the way to give several PRs the chance to run against the new tests.