LEA_mDNSResponder #5358
LEA_mDNSResponder #5358
Conversation
There was a problem hiding this comment.
Very cool, @devyte should be insanely happy. I'm a little worried about the timeouts based on (millis()+x) and wraparound and identified the spots I think I see potential wraparound problems.
| /*LEA*/ | ||
| //Serial.print("\nClientContext::connect (A)"); | ||
| uint32_t nextPrint = (_op_start_time + 100); | ||
| while ((millis() < (_op_start_time + _timeout_ms)) && |
There was a problem hiding this comment.
Can't do millis() compare like this. Need to subtract now from the start millis and compare to the timeout value. OTW you'll end up with bad things when millis or millis + timeout rolls over.
There was a problem hiding this comment.
Ohhh... the ClientContext changes wasn‘t supposed to be part of this... sorry!
| while ((millis() < (_op_start_time + _timeout_ms)) && | ||
| (state() != ESTABLISHED)) { | ||
| yield(); | ||
| if (millis() > nextPrint) { |
There was a problem hiding this comment.
Same millis issue. Plus, I'm not sure what the purpose of this as we're not going to be printing anything anyway...
Would suggest going back to the delay(_timeout_ms); for this whole section.
There was a problem hiding this comment.
Same as above, this is just a q&d solution for the connection problems with the existing implementation! Wasn‘t supposed to be committed here, sorry! Just remove if possible!
There was a problem hiding this comment.
I find this interesting. Does the current implementation have an issue? Does this resolve it?
There was a problem hiding this comment.
The current implementation relies on a break of the delay based on a call to _connected. When experimenting with BearSSL to create an Apple Push Notification Service, I found, that the Client::connect call stalled the whole system (without any timeout) because the supposed break of this delay never happened... My q&d workaround solved this problem for me...
| LEAmDNS::MDNS.update(); | ||
|
|
||
| // Update time (if needed) | ||
| static unsigned long ulNextTimeUpdate = UPDATE_CYCLE; |
There was a problem hiding this comment.
Same issue w/millis wraparound here, I think.
There was a problem hiding this comment.
I‘ve only now learned, that a millis() roll over happens every 72 minutes... Is there any alternative way? Maybe some tick counter??
There was a problem hiding this comment.
I’m getting confused ... is the millis() rollover really hard-connected to the micros() rollover?? Otherwise it would be a rollover time of about 6 weeks... (haven’t seen an ESP running that long without running out of memory and needs a restart :-( )
There was a problem hiding this comment.
@LaborEtArs millis() does a rollover, so you can't compare the value returned by millis() against a delta. Instead, you have to compare a delta against a value. A delta (i.e.: a millis() - op_start_time) considers the case of rollover, so the result is always correct if you happen to be caught in the middle of a rollover.
Well, "always correct" as long as you don't have 2 rollovers in between 😛 .
This kind of bug, and having to think your way through it every time you need to do a timeout operation, is the motivation behind #5198 : have the details abstracted and simplify coding.
But it's not merged yet.
There was a problem hiding this comment.
Ok, got the problem!
I guess, I will incorporate a similar class as your pooledTimeout here.
BTW: There had been a short discussion about the types and type names in that thread. Personally I really don’t like the use of ‘using ... unsigned int’ for standard types, as it extremely reduces the readability of the code. Of course not, if there are just one or two of those ‘using’ declarations, but the more the worse it becomes. I would plead for standard types like uint32_t in cases like this.
| (_getResponseMulticastInterface(SOFTAP_MODE | STATION_MODE) != IPAddress())) { // Has IP address | ||
| DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(PSTR_LEA("[MDNSResponder] _updateProbeStatus: Starting host probing...\n"));); | ||
|
|
||
| // First probe delay SHOULD be random 0-250 ms |
There was a problem hiding this comment.
Millis wraparound again.
| for (stcMDNSService* pService=m_pServices; ((bResult) && (pService)); pService=pService->m_pNext) { | ||
| if (ProbingStatus_ReadyToStart == pService->m_ProbeInformation.m_ProbingStatus) { // Ready to get started | ||
|
|
||
| pService->m_ProbeInformation.m_ulNextProbeTimeout = (millis() + MDNS_PROBE_DELAY); // More or equal than first probe for host domain |
There was a problem hiding this comment.
And here...anywhere endTime= (millis() + XX); while (millis() < endTime()) {}
|
|
||
| #ifdef USE_PGM_PRINTF | ||
| // See: https://github.com/esp8266/Arduino/issues/3369 | ||
| #define PROGMEM_LEA __attribute__((section(".irom.text.LEA"))) |
There was a problem hiding this comment.
PROGMEM variables are now uniquified, is this still required?
There was a problem hiding this comment.
I‘ve got linkage problems without this...
There was a problem hiding this comment.
I think the unique sections haven't been released yet and are only available in latest git. I also suspect that @LaborEtArs is really working on a hand-modified 2.4.2 code base, which means the unique sections aren't available in his build.
Currently by adding a small helper class which does the timer handling.
Added the 'nullptr' changes from #5342
|
Closing this PR. Created a new branch to solve commit conflicts with recent changes to ESP8266mDNSResponder.cpp by d-a-v. Will open a PR there. |
First try :-|