FDRS Support for timekeeping - 2024 version #193
Conversation
src/fdrs_gateway.h
Outdated
| @@ -118,6 +120,12 @@ void beginFDRS() | |||
| Serial.begin(115200); | |||
| UART_IF.begin(115200, SERIAL_8N1, RXD2, TXD2); | |||
| #endif | |||
| #if defined(USE_OLED) || defined(USE_DS3231) || defined(USE_RTC_DS1307) | |||
There was a problem hiding this comment.
Typo - need to change USE_DS3231 to USE_RTC_DS3231 here.
src/fdrs_gateway_lora.h
Outdated
| SystemPacket pingReply = {.cmd = cmd_ping, .param = 1}; | ||
| transmitLoRa(&sourceMAC, &pingReply, 1); | ||
| else { | ||
| DBGF("LoRa 0x" + String(sourceMAC, HEX) + " is not time master, discarding request"); |
There was a problem hiding this comment.
Should be DBG and not DBGF
src/fdrs_gateway_lora.h
Outdated
| SystemPacket sys_packet = {.cmd = cmd_ping, .param = 0}; | ||
|
|
||
| transmitLoRa(address, &sys_packet, 1); | ||
| DBGF("LoRa ping sent to address: 0x" + String(*address, HEX)); |
There was a problem hiding this comment.
DBG and not DBGF
|
Well I'll confess, the CBOR thing has more facets than I first considered. That will be on hold for at least a week while I think, fyi. |
|
I'm not sure what CBOR is. Haven't look into it yet and haven't heard the guy with a Swiss accent reference it yet. |
|
From the end of the previous timekeeping PR:
It's nothing too exciting, just a data format. I'd been planning to try to hammer it out this weekend and just letting you know that this isn't happening so soon. |
|
For sensor nodes that power up, collect sensor data, send sensor data, power down, no need for them to know the date and time, right? |
Right. I don't see why it would be required there. If the user runs I'm still reading through the new code, but I think it's great so far. I forget how it was before exactly, but the new datatypes and time master concept look like a good move. |
|
@timmbogner question for you in the scenario where we have a controller using ESP-NOW. It seems that the controller registers itself to the gateway when we call the addFDRS() function. This function is used to register a call back function in case we want to send data to the controller. However, what if we have a controller where we do not need to register a call back function? Maybe this is really the definition of a controller where addFDRS is really required but I'm thinking that maybe we decouple the registration part from the addFDRS function call. Every controller should register to the gateway even if addFDRS is not called, if I understand correctly so maybe better just to register automatically even if addFDRS is not called in main.cpp. So if you are in agreement then all addFDRS would do would register the callback function. The registration of the controller with the gateway would be called independent of addFDRS. Additionally, if the registration with the gateway fails, I would think that we would want to periodically try to reregister with the gateway. Then I would wonder if we have a sensor node that does not go to sleep, we should probably register the sensor with the gateway and thus the gateway would send the sensor the time which makes sense to me if the sensor is continuously powered on. What are your thoughts on that? Finally, in the "node" code there is quite a bit of ESP-NOW and LoRa stuff in the fdrs_node.h file that I think can be moved to the fdrs_node_espnow or fdrs_node_lora files. If you agree, I think I'll take up organizing that code but in a different PR. I think the ESP-NOW controller code is good so now to work on LoRa controller... |
You kind of called it right there. If a node is receiving data, it will need a callback in which to handle it. If it needs a callback then it will need to be defined somewhere. Thus, I see now that in LoRa, In nodes,
I think in this case, if the user wanted the sensor to get the time, they could register as a controller. I do see how it might be nice to avoid requiring a registration spot just for a clock, though. I still need to look deeper into your code, so take all of this with a grain of salt. I haven't fully wrapped my head around it at this time, but that's just because I've been distracted, not anything to do with the code itself. I'll give it attention today for sure.
Agreed. I'm not sure if that stayed there for a reason or not, but |
|
Jumping in to testing. I'm using Arduino IDE 2.3.0 and ESP32 board defs 2.0.14: 0_MQTT_Gateway works great, gets the time. 1_UART_Gateway with ESP-NOW gives the following error: 1_UART_Gateway with only LoRa enabled gives the following error: |
|
If the callback function was fixed at a certain name then can't we do all of that automatically without having them call the addFDRS() function? Then all we would have to do is check if the node was a controller or a sensor. Could we just call a node that doesn't define DEEP_SLEEP as a controller and one that does define DEEP_SLEEP as a sensor? Then use that as a condition for registering the callback function and registering. It seems at this time there can only be one callback function. Maybe you've thought of this? Regarding the fdrs_node.h code I may take that reorganization as a task at a later point. |
I've been thinking about it more since you mentioned it and you're right, it could be static. I think a problem with what you say is that each ESP-NOW device that registers as a controller takes up one slot in the peer list of the gateway, which is limited to 16. It would be best to keep these to the devices that actually need to receive data. Shouldn't there be a way for a node to request the time? I guess this is sort of what you asked about early on. I think the way it should work is: If a node connects as a controller, it will get time updates from the gateway. Any sensor can send a request for the time to its gateway and get the time that way, but it won't be fed the time automatically. In general, I think a sensor should not be listening or receiving anything unless the user explicitly runs As a side note: The controller callback is going to come under more scrutiny soon, as I trudge through this CBOR stuff. If the incoming data can be one of several types, will there have to a callback for each? Or will there a function within a single callback to decode it? Will there be more variables passed to the callback? It doesn't help that despite using them a few times, I still need to review every time I work with function pointers. I've tested ESP-NOW Gateways 00 -> 01 ->02 plus a controller, and they all are working perfectly and keeping time. LoRa next. |
Sounds good. makes perfect sense.
I'll look into this. Shouldn't be hard to do this. I think the code is pretty much there. Just need a good function call.
Sounds good. I'll leave that stuff alone.
I have to do some more testing in edge cases and different settings so I'll post more updates as I find issues. Sounds like we're going to make the assumption that the MQTT gateway is the only gateway that would be the initial source of time, right? |
No, my intent is that any gateway could be a source of the time. Hopefully that's attainable here. Let's take a case where there is the same gateway scenario as the examples.
I want to make sure this feature is still available if MQTT isn't used on the network. For example if the front-end is just a logger using serial and no WiFi. I'm sure I'm overlooking some things still... |
|
So, using your example, let's say 00 does not have Internet access but does have RTC. Somehow, the time on the RTC is off and not correct. 02 has GPS. GPS should be preferred over RTC. Do we implement a check if the two time sources differ use the one that should be more accurate? 01 would be able to tell if the time they are receiving from 00 and 02 are different by some appreciable amount. |
I'm thinking that we make the assumption that if a gateway has an RTC that it should be updated at fairly regular intervals by some Internet connection or GPS otherwise why put the RTC on that gateway. So I'm going to then assume that the RTC and other gateway with GPS are assumed close enough in time to each other. If there is an issue where the time is off (> 10 seconds?) then raise a DBG message and hope that someone sees the message. Thoughts on that? |
|
Yeah I'd been thinking about RTC too. I think I may have come up with something just now. What if the RTC gateway never shares its time until a time is sent to it? Basically we assume the RTC time is incorrect until an outside time packet sets it. If the user wants to pre-set their RTC and use it as the only time source, we can set up a config to override this behavior. So... what am I forgetting? |
|
Is it possible to paste a timestamp into the serial console and set the time manually? |
I didn't see this til just now. I think we're thinking pretty closely. I had also thought about a routine that keeps track of whether incoming times are synced. I like the idea of a debug message, and we could expand from there if wanted. |
|
Periodically generate a FDRS message indicating a time issue so it can be sent to the front end. Like an alarm condition. |
Excellent idea! We can use |
src/fdrs_gateway_serial.h
Outdated
| DBG("Incoming Serial: DR"); | ||
| String data; | ||
| serializeJson(doc, data); | ||
| DBG("Serial data: " + data); |
There was a problem hiding this comment.
JSON data output to the console should be displayed regardless of FDRS_DEBUG. Among other things, it's documented or mentioned somewhere to work that way, so we shouldn't change it. The idea is that the console interface always doubles as a data interface.
There was a problem hiding this comment.
Timm, I'm not sure I understand that comment. Can you elaborate a bit on:
1." JSON data output to the console should be displayed regardless of FDRS_DEBUG."
2. The idea is that the console interface always doubles as a data interface.
Are you saying that even if the DBG is not there we should see the data on the console?
There was a problem hiding this comment.
Yes. Even if FDRS_DEBUG is disabled, the "Serial" (as well as "Serial1") interface should print out DataReadings when sendSerial() is called. I also think it best if they are delivered surrounded by line-breaks. Just in case a front end application is ingesting from that interface and needs it more standardized. Does that clear things up?
It was originally implemented due to the ESP8266 having only one serial interface. If you need/want to re-use that interface for GPS or something, maybe we could have a way to explicitly disable this. Afterthought: A way to fully disable the serial interfaces might be called-for if we need that.
Side note: I might disable the new indentations in the debug console when at level 0. The blank space is just a bit jarring (probably just to me) but it's also easier to read when info is closer to the timestamp.
In my mind this depends upon the regulatory requirements (laws in the area), how reliable longer transmissions are as compared to shorter transmissions, and what, if any, loss in control this has on the micro controller. For the latter, the longer the transmits are the more execution needs to be done when the transmission completes. The more features that are added to the project the more critical it will be to "service" those features in a more timely and regular manner. While I was doing that stress testing it seems like 10 DRs was taking about 200ms of transmission time. It doesn't seem like that would be prohibitive at this point. |
|
I think a point we're differing on is how many DRs is "a lot". As users scale up their systems, there could be a load of 30+ DataReadings arriving at a gateway at a time. Currently, I simulate this by inserting 30 JSON DataReadings into the console of 0x01, and send them via ESP-NOW to an 0x02 ESPNOW/LoRa gateway. The response when trying to re-transmit them with LoRa is as follows: For your copying convenience, here is a sample array of 30 json DRs: |
Sounds good, so I think the first thing to do then is make the queue/buffer larger. In fdrs_lora.h line 14 I make the buffer size 250/7 = 35. We can definitely make that larger to something like 70 pretty easily by changing that line 14. This will clear up the buffer full messages. Then we have the issue of limiting the amount of LoRa data transmitted to the maximum data size of the LoRa packets. I believe this is around 255 bytes or about 35 DataReadings, if I recall correctly. I don't believe there is any limitation on the number of DRs to be sent (size is len) so we should probably add some code to check for the upper bound of len > 35 so that we only send 35 DataReadings at one time. I guess this could be done in the transmitSameAddrLoRa() function (fdrs_lora.h line 380) at the return statement: Edit: 250/sizeof(DataReading) is also called LORASIZE so use this instead: |
|
Okay I just realized ESP-NOW actually isn't splitting large packets correctly either. I'll need to look at that. BUT 😅 If on the main branch you go directly from serial to LoRa, and you paste in 36+ DataReadings, you will see it split the data into two packets and send them sequentially as follows. This behavior needs to be kept up. I'll look into ESP-NOW. |
I believe ESP-NOW does handle this. This limits the transmission size to maximum ESP-NOW data transmission size.
Yep, for sure |
Ah, I was still testing in the main branch, so I guess you fixed it. Thanks! I'm still digesting the changes to ESP-NOW, so bear with me. I'm going to commit the adjustments I have made so far, then continue testing the actual time portion. |
Another circle-back: Prototype: int pingFDRS(uint32_t timeout)
{
global_ping_flag = false;
#ifdef USE_ESPNOW
pingFDRSEspNow(gatewayAddress, timeout);
#endif
#ifdef USE_LORA
pingRequestLoRa(gtwyAddress, timeout);
#endif
while (!global_ping_flag){
yield();
handleLoRa();
}
// do some stuff to get the response time
return response_time;
}There will need to be something add to handle timeouts, but I think this illustrates what I'm hoping to do. |
I wrote the following over several hours so it may be a little jumbled up... I think the issue is not the ending part of the synchronous or blocking function it's the beginning part of it. Once it's called we can execute it without issue it's just when calling it we need to make sure there is no async function already running. That's why, in the LoRa ping code, I check to see if we are waiting for an ACK and if so, instead of running the ping right away, I queue up the ping for later execution. If we didn't check for that we might miss the ACK and the ping response both. If we ran the following code in a node.. There are at least 4 LoRa transmits there. When in that order there should be no issues running the synchronous code, but what if we ran it in this order. If everything was synchronous and we are using ACKS then we may miss the ACKS during the ping, subscribe, and time request. The challenge when using both synchronous (blocking) and asynchronous functions is that when you call the synchronous function it may not be able to do what it needs to do right away due to an asynchronous function already in process. So, for example, in LoRA, the transmissions of the DataReadings when using ACKS are asynchronous (if they were synchronous they may block for like 1 second or so). If they are ongoing and then there is a call to a ping which is synchronous then we either cannot execute the ping or we have to wait and execute it later after the asynchronous task is completed. With LoRa this is only complicated when ACKs are enabled as those require listening for responses and then potentially transmitting again. So what I do in the ping and time request functions is to check to see if an Asynchronous task is running and if so I would queue up the ping for transmission later. If you call this ping function that could be asynchronous there is no guarantee that it will return a value. What do you think about a callback function where when the ping completes, a function is called that can print to the output the ping response time and also hand that value to other code if you want to do something else with that value? I see what information you desire but I'm not clear why it's needed as a response to pingFDRS and a callback function can't be used. It might help if you could clarify what you're going to use the ping response time for. I think pingFDRS returning a value makes everything clean and easy to understand. Maybe what we could do is return a response value when the ping is performed synchronously and then just return 0 or some very high number when the ping is performed asynchronously.
I think that's the magic of using ESP-NOW and the second core. I think it generates an interrupt saying that there is a ESP-NOW response and then calls the callback function therefore that works when you're spinning in a while loop waiting for a response.
If you're spinning in a while loop in pingFDRS waiting for a response I don't think there's any other way to execute other code in general and wait for a response at the same time. I think a callback function would work but I'm open to doing what you think. |
|
I've realized that this ping stuff is a relatively minor feature, and the main reason I want to keep it is because it already exists in the old version. In reality, I must admit that being able to actually use the ping time is a fringe-feature. Few people will need it, although it is nice to have. I think the callback is just too much extra stuff for too little real benefit. So basically, I don't care too much about keeping the ability to use the ping time. Displaying is what most users will need, if they even use the ping functionality in the first place. You are welcome to implement your callback idea if you want, as long as it doesn't add much config overhead. Sorry for flip-flopping here! |
|
Sounds good, I'll fix up the ping stuff today (making them return int - both ESP-NOW and LoRa) and then do some more testing. |
…f, check for LoRa pkt overflow
|
Working on web page to set time, change debug level dynamically at runtime, and show debug output.
|
|
I really like the webpage thing. Did you have to change DBG level from a macro to be configurable after compile? I think I should have time for testing in the next week. After that I'll be on the road until roughly the end of the month. |
I was thinking about that as well. Would be a lot of work but this makes it easier.
I think I Just wanted to show you what I'm up to next. I think I'd rather be done with this PR and keep this to the next PR.
Sounds good, no rush. I don't think I'm planning on submitting any more to this PR unless I find bugs. Edit: |
|
I'm still here! When I left off a few weeks ago, the gateway functionality was tested with no issues. I will be testing nodes in the next week or so. |
also small tweaks to ping debugs
|
@aviateur17 I finally solved the conflicts; maybe even correctly! It wasn't as painful as I'd anticipated. Please check over things to make sure I didn't unleash any unanticipated chaos 😅 The last thing I want to do is create a simple example of how to use the time data. The T-Watch sketch is a bit convoluted. I'm thinking just something that delivers each element of the time individually over the serial monitor. This can act as a framework that a user can adapt to a TFT/7-segment/OLED or whatever kind of clock display they desire to build. If you have anything like that ready to go, or you'd like to contribute this, obviously please go for it! I plan to do it sometime, but I am not going to delay this PR any further either way. As long as this merge didn't break anything, I think the PR is ready to go! |
|
Timm, sounds good. It may take a couple of weeks as I don't have a lot of time right now but I'll check it out and make comments to keep up to date. |
|
Output from Controller Time example to be submitted shortly: The timestamps in front are my personal added code so won't be shown. Edit: Added daylight savings time (shown in below output). |
|
Timm, the merge seems to be good based on my review of your commit. I think there may be an issue from my doing in the node_espnow code in the pingEspNow function. Let me review that and make sure everything compiles properly. |
|
Timm, I just made the changes mentioned earlier. I think we're good to go. |
|
I'm going to merge this (sorry it's taken so long), but I still need to add to the documentation. I'll get to that once my springtime work dies down. I noticed that LilyGo has a new T-Watch with an integrated LoRa radio. I'm excited to order one, and I'd love to send you one too as a token of my thanks for everything you do for FDRS. Email me if you're interested! |
Add gateway time keeping, NTP, RTC support
For Gateways, add time keeping in all nodes - MQTT, serial, ESP-NOW, LoRa.
Add support to fetch time via NTP if WiFi is enabled.
Add support for I2C RTC devices - DS3231 and DS1307 and compatible
Gateway will only send time out via serial if WiFi or RTC is in use to prevent loop. Don't want serial node to send time back to WiFi/MQTT node.
Priority in Time master Serial > ESP-NOW > LoRA which means a gateway that is receiving time via serial will reject time via ESP-NOW and LoRA. A gateway that is receiving time via ESP-NOW will reject time via LoRa. If no time is received after 60 minutes then the "master" time server will reset to none.
Automatic transition between daylight savings time and standard time for both US and Europe.
Still need to work on support for controllers and sensors in a future merge
Will also verify that compilation works in Arduino environment (I am using VSCode and PlatformIO)
Also need to verify compilation against different platforms such as ESP8266, raspberry pi Pico, SAMD21. What other micro-controllers are intended to be supported?