Skip to content
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

Device Groups Multicast Address #8258

Closed
pcdiem opened this issue Apr 23, 2020 · 3 comments · Fixed by #8270
Closed

Device Groups Multicast Address #8258

pcdiem opened this issue Apr 23, 2020 · 3 comments · Fixed by #8270
Labels
enhancement Type - Enhancement that will be worked on

Comments

@pcdiem
Copy link
Contributor

pcdiem commented Apr 23, 2020

Have you looked for this feature in other issues and in the docs?
Yes

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is.

When I wrote device groups support, I chose to make use of the existing SSDP UDP multicast code used by emulation support in the interest of code size. This can cause conflicts with overly sensitive UPnP devices and forces devices with emulation otherwise disabled to receive all UPnP packets received from the network. Device groups is not UPnP and really should not be using the SSDP multicast address.

Describe the solution you'd like
A clear and concise description of what you want to happen.

If I can get a 32-bit setting allocation, I'll submit a PR to make the device group multicast address configurable.

We could make the default device group multicast address be the SSDP multicast address (239.255.255.250). This would enable this to be a non-breaking change but would require that the emulation code continue to check for device group messages. It would be best if users migrated off the SSDP multicast address so we could eliminate the extra code. The migration is unlikely to happen though unless there is a breaking change that requires users to upgrade all devices using the device groups feature. Even new users would likely leave the multicast address at the default SSDP multicast address.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

I've made the modifications to device group support to use a dedicated multicast address defined at compile-time. The modifications add 175 bytes to the binary. I also have modifications to support using the dedicated multicast address while still supporting devices that use the SSDP multicast address which add 268 byes to the binary. This would allow users to continue to use 8.2.0 firmware. At some point in the future we could make the breaking change to remove SSDP multicast address support.

Overall, the device groups feature has only been available since the latest non-development release (8.2.0) and the number of users using it is relatively small. For this reason, I'd be in favor of making a breaking change that would require all devices using the device group feature to upgrade.

If we go with the breaking change, the device groups multicast address setting would be a nice feature but not necessary since the compile-time setting is not the SSDP multicast address.

Additional context
Add any other context or screenshots about the feature request here.

(Please, remember to close the issue when the problem has been addressed)

@pcdiem pcdiem changed the title Deveice Groups Deveice Groups Multicast Address Apr 23, 2020
@pcdiem pcdiem changed the title Deveice Groups Multicast Address Device Groups Multicast Address Apr 23, 2020
@Jason2866
Copy link
Collaborator

My 2 cents. Let´s do a clean fix (with breaking change) now.

@arendst
Copy link
Owner

arendst commented Apr 23, 2020

Agree

@ascillato2
Copy link
Collaborator

👍

@ascillato2 ascillato2 added the enhancement Type - Enhancement that will be worked on label Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Type - Enhancement that will be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants