Skip to content
This repository has been archived by the owner on Nov 17, 2020. It is now read-only.

Small corrections #1

Closed
wants to merge 5 commits into from
Closed

Small corrections #1

wants to merge 5 commits into from

Conversation

valinet
Copy link

@valinet valinet commented Nov 14, 2020

Hi

Thank you very much for this piece of code, it is an excellent starting point for implementing mDNS with the ENC28J60 and being so lightweight it is so easy to understand and customize it according to each project's needs.

I have made a few small changes to the code which I think would be worth including upstream.

First off, I modified the includes to contain relative paths, so that it works when the files are alongside the sketch file, as well as when the library is traditionally placed in Documents/libraries folder.
Then, I modified the signature of the callback for ether.udpServerListen (onUdpReceive) to accomodate changes in latest versions of EtherCard (the source port is provided as a parameter as well now).
Lastly and more importantly, for this to work reliably, I believe the default behavior when replying to a query should respect the mDNS RFC 6762 (https://tools.ietf.org/html/rfc6762), specifically this mention: "The destination UDP port in all Multicast DNS responses MUST be 5353, and the destination address MUST be the mDNS IPv4 link-local multicast address 224.0.0.251 or its IPv6 equivalent FF02::FB, except when generating a reply to a query that explicitly requested a unicast response". Most clients are doing multicast queries when looking for "some-name.local", so according to the specification, the reply should be sent to the mDNS multicast address as well, not to the IP that originated the query. A small patch should be implemented in EtherCard as well (in EtherCard::udpPrepare), in that the MAC address in the Layer 2 header is not populated correctly: it is set to the standard IP broadcast MAC address for both broadcast and multicast addresses, whereas for multicast addresses it should be set to the standard IP multicast MAC address (01:00:5E:00:00:FB), but this is out of the scope of this commit; in practice, it works as well without modifying EtherCard, I have tested it with requests made on Chrome (Windows 10), and Safari (iOS 14).

That's it, thanks again for this wonderful code, hope you are well.

Valentin-Gabriel Radu added 4 commits November 13, 2020 20:30
…ests), it should be sent to the mDNS multicast address, not the requesting IP, as per RFC6762.
…n the EtherCard buffer, in order to dramatically reduce memory usage.
@itavero
Copy link
Owner

itavero commented Nov 15, 2020

Thanks for your PR.

Unfortunately I haven't worked on this for about 7 years and I'm no longer actively using it.
I also don't have the time not hardware to verify your changes at the moment.

I'm wondering if it would not be better if I'd archive this repo and you'd keep your fork alive.

@valinet
Copy link
Author

valinet commented Nov 15, 2020

Hi

Yeah, this may not be such a bad idea, in the mean time, I added some more changes that optimized the memory usage a bit (migrated some constant arrays to PROGMEM, generate the response payload on-the-fly opposed to ahead of time so that some memory is spared). I am actively using this in a home automation/control system, as you imagine, I am quite tight on the mmeory budget so any reduction/optimization is well appreciated, so if I find stuff worth improving I will push those changes as well. So if you are okay with it, I don't mind making my fork the default repository and me maintaining it.

Thank you very much for your reply, looking forward to hearing from you.

* This library can now advertise the device in the local network via
  DNS-SD; support for replying to all service queries, a particular
  service query, and a particular instance query is implemented
* Updated README and example to reflect recent additions.
@itavero
Copy link
Owner

itavero commented Nov 16, 2020

GitHub made your repository the root (aka main repository).
I'll archive this one.

Good luck with the further development! 👍

@itavero itavero closed this Nov 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants