-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
mDNS not showing all TXT records #248
Comments
I don't know much about mdns and I don't know if this is related, but comparing different esphome implementations of MDNSComponent, there's one thing different with libretiny: lack of the loop() method, which other implementations seem to use to call update() periodically. class MDNSComponent : public Component {
public:
void setup() override;
void dump_config() override;
#if (defined(USE_ESP8266) || defined(USE_RP2040)) && defined(USE_ARDUINO)
void loop() override;
#endif @kuba2k2 Is this by design or was simply forgotten? Could this be related to @chrwei observerations ? |
It's not related - the loop() method was added by ESPHome after LibreTiny had mDNS working. It's most likely related to RP2040 support which needed that method. I think it's used to call some mDNS callbacks that need to be called periodically, if any. As to why the original issue happens - I'm not sure. Maybe there is some hardcoded length limit that just breaks things when exceeded. It will need further investigation. |
that's what i thought too, but unless there's a (char *)malloc limit I don't see one. |
anyone able to reproduce this? |
Quickly looking into this, 44 is the length of the value you're adding into a TXT record, but it looks like the issue is when the full record length is 64 or more characters, which is a much more logical number from a coding perspective. In your own example
the first would have a value of It does appear I have a few devices that are not broadcasting mdns as well, but I am unsure of why at this point, I don't believe I have any fields that exceed a total length of 63/64. Uncertain if it is related or not, but I do find |
I hadn't though of that, and that would overflow an int8 length, I just don't see anywhere that would be the issue |
A little further investigating, neither 44/45 nor 63/64 appear to be the issue. I had no field that long, but I shorted one of my longest strings until I started receiving a packet. Then I shortened a different TXT record, and I was able to expand the one I previously had to shorten by that same amount. It seems the maximum data length of all TXT data is capped at 213 bytes, which again, is a very unusual/non-programmatic number. |
I did find this in some lwip multicast mdns documents ( https://www.nongnu.org/lwip/2_0_x/group__mdns.html )
However, that doesn't seem to match what I'm experiencing in my own testing, as I don't have any single record including length (which is 1 byte) that is anywhere near 63 bytes, longest is 56 + length byte, so now I'm not sure what to think. |
Since I don't know much about mdns, I decided to try this from a different angle.
The above log tells 2 things:
@chrwei if we setup all your strings in MDNSComponent::compile_records_() exactly how they appear in your device, we might actually be able to debug this issue. As long as it is reproducible on a PC :) But at least we'll know. Attached is (almost) all that is needed to build and run this test. Additionally you need to install liblwip (sudo apt install liblwip-dev). |
Ok, after a lot of debugging and chasing ghosts in the code I came to the same conclusion as you guys: this is an issue with hard limit in lwip implementation - max 64 characters for single txt label (length field not included).
Hey, I've learned something today :) |
We could include a fixup for |
That might work providing lwip always use this constant consequently - otherwise it's risky. I would first ask the authors opinion before patching it like that. |
I've just read in the RFC that this limit is imposed by the specification... so shouldn't this also NOT work with other implementations? |
I haven't test your changes yet, but that's just it, none of the strings I was testing with personally should have even been hitting that 63 character length. So it's possible I was running into the underflow error, and OP is running into the max length issue. I honestly don't even know the difference between what is a label and what is a name, I have little knowledge of the core of mDNS. Edit: |
The more I test, the more I think we were just running into random underruns that would just kill the entire mDNS send (because of the loop break/kill/return instead of just skipping the single TXT record). The value I was testing with before (which in the TXT key=value format was only a length of 59 and was previously failing) is now working, and the only thing that has changed in the mDNS packet (that I am aware of) is the ESPHome version string in a different TXT record. |
Unfortunately, it looks like the underflow was not our issue. I tried deploying it to a problematic device, and it is no different. For me, the only reproducible case is when the data size of all TXT records combined exceeds a data length of 208, the entire mDNS packet is dropped/not sent. And it will begin to fail if I add any characters into either And it's not like we're running into packet size issues, as the entire mDNS packet is only 475 bytes when the TXT data length is 208. None of my individual TXT records are exceeding 63 characters, so that's not the limiting factor either. Edit: my previous testing the overall data length was 213. Not sure if other non-TXT records come in to play or not. It does seem like I call the overall packet size being 475 then as well (may have also been a different device). |
Which brings up, what's esp8266mDNS doing different that the same ESPHome config works on esp8266 devices? lwip seems to try to shove it all into one packet, but it is separate TXT records, so I'm not sure why that matters. wireshark capture zip file (2KB) https://www.dropbox.com/scl/fi/gx8n34zqkvf3spbje2pxs/mdnscap.zip?rlkey=gju38y2ejj2oqjszva2ck7d4t&dl=0 .232 is the esp8266 and .181 is the bk72xx. capture is power on one, then power on the other. |
I'm not fully convinced it's related to packet size either, as the initial packet is large, but subsequent packets sent later are smaller (on devices that don't have this issue). The issue on LibreTiny is if it doesn't send the first packet, it also never sends any subsequent packets, so it appears the entire mDNS module fails and stops. I've been testing remotely so far, without serial, so I haven't had a chance to enable LT debug logs yet. I'll try it on a dev board with serial and see what I can find there. |
Serial logging only gives one more tiny clue, but not smoking gun like an error like I was hoping. The following is the full LT mdns output when the packet is not sent:
and the following is the output when the packet is sent:
So we get 2 status lines at the end if successful, but nothing if not. |
yeah you'd need to add more debug output to the code. the MDNS.addServiceTxt(service_type, proto, record.key.c_str(), record.value.c_str()); call is the issue, so the extra debug would need to be inside that addServiceTxt method to see where it's "crashing" |
The issue isn't in addServiceTxt, but rather the callbacks I modified mdnsTxtCallback to log every time it called mdns_resp_add_service_txtitem Here is the output from a working case (cleaned up)
It hits the status, and then the callback ceases to be called. Then when it resends packets, it goes through a loop and promptly stops each time. For the non working case, it never breaks and seems stuck in a loop in mdnsTxtCallback
This is truncated, but it would keep going on forever. |
I think it's just hitting the LwIP mDNS limits from mdns.h:
I think those are too small. RFC6763 §6.2 says:
I can increase |
Turns out I can increase MDNS_LABEL_MAXLEN to 314, but not 315. It's actually the size of the |
I'm using a bk7231n cb2s based device with esphome and had already opened an issue there esphome/issues#5435 but it seems the issue is in libretiny's mDNS
to summarize the issue linked, when esphome adds a TXT record that is longer than 44 characters, that record, and any TXT records added after that, do not show up in the mDNS packet. They do show up in the esphome logs, so it's somewhere in how libretiny's mDNS handles the
MDNS.addServiceTxt(service_type, proto, record.key.c_str(), record.value.c_str());
call.I've looked at the code and I don't see anything obvious that would cause this, but my C++ skills are basic.
The same yaml works on esp8266, so it's not a non-printable character, I even retyped the line to be sure. I did attempt to compare the esp8266mDNS code, but it's too different to make a useful comparison.
The text was updated successfully, but these errors were encountered: