Make server ports non-reusable and NetworkServer config more flexible #721
Conversation
Now takes addresses as a list of listening address instead of one each of IPv4, IPv6 and BLE. Adds support for dual-mode (IPv4 + IPv6) sockets. Adds one dual-mode UDP listener on all addresses if none is configured. Adds one BLE listener if BLE is supported and "ble" option is not set to false. Throws an error if there are multiple BLE addresses or the HCI address is specified in the listener list (these require lower-level changes I did not make yet). I want to make similar changes for Mdns at some point but figured I'd see how these go over first. Includes some additional convenience methods for variable access.
|
Oh yeah I forgot, this also includes changes to UDP API so that we don't turn REUSE_ADDR sockopt on by default but only when necessary. This prevents users from accidentally starting multiple servers on the same port and is actually why I was in this code in the first place. |
lauckhart
changed the title
lauckhart
changed the title
| hasBle = true; | ||
| } | ||
| if (address !== undefined) { | ||
| throw new NotImplementedError("Currently you may not specify HCI ID for BLE transport"); |
There was a problem hiding this comment.
We need to have HCI support here because else shell will have big issues. in some cases you need to use a secondary adapter... Can we please add this directly? i wanted to migrate shell also soonish to new API basically
There was a problem hiding this comment.
PS: why not define hci id as "address" in this case?
There was a problem hiding this comment.
That's the goal but will require refactoring Ble.ts to convert Ble from a singleton and add HCI option. Then we need to make it so multiple servers that share the same BLE address can share the broadcaster. We need something similar for the MDNS side of things so figured it might make sense to do that first.
So anyway, thought this was a good intermediate place to stop.
There was a problem hiding this comment.
Maybe lets have a chat about that ... current realizy on nodejs and also on mobile devices is that there is in fact one BLE radio to use ... also bleno/noble is kind of limited to one right now and weould need a rewrite to make that configurable. Multiple servers using the same BLE radio in fact make not sense at all ... that would only be a topic when bleno would be rewritten for such a support (there was such a PR somewhen but would be need to revamped completely). So in my eyes ble can only work for one of the Server/Controllers on the Host right now.
Ok for MDNS I thought we have whats needed already, but ok maybe I miss something ... so maybe lets have a chat what you plan/imagine...
packages/matter.js/src/behavior/system/network/NetworkServer.ts
Outdated
Show resolved
Hide resolved
| * | ||
| * Configurable also with variable "network.listen". You may configure a single listener using: | ||
| * | ||
| * * `network.listen.protocol` either "ble", "udp4", "udp6" or "udp" (default is "udp" for dual IPv4/6) |
There was a problem hiding this comment.
we need to see where we document all these ... I started as main comment also on Environment (in this case the NodeEnvironment). So we need to update there.
There was a problem hiding this comment.
I think in a separate .md file would be ideal, I've been planning to document in the code near where the options are used. Environment seems to generic to document the vars, many of which will be irrelevant if you're a controller vs. device.
This was one of the things I was going to address in the documentation push.
|
|
||
| transports.add(udp); | ||
|
|
||
| // We advertise the first (most likely only) UDP port we open as our official address |
There was a problem hiding this comment.
I would notz allow different ports at all, if not announced noone will use it ... so honestly I see no sense in allowing different ports
| break; | ||
|
|
||
| case "ble": | ||
| // TODO - HCI ID and/or multiple BLE transports? (Former seems valuable, latter not really but |
There was a problem hiding this comment.
just a single BLE is making sense. forbid multiple
There was a problem hiding this comment.
... and see above.formally only one BLE per host ... soooo multiple servers the server should define if ble should be (not) added too?
| } | ||
|
|
||
| // AFAICT no way to query transport interface for protocol type so just store BLE transports separately so we | ||
| // can identify them for removal after commissioning completes |
There was a problem hiding this comment.
BLE is by definition commisioning only
There was a problem hiding this comment.
Yes, this is so we can identify which ones to remove
There was a problem hiding this comment.
was mainly regarding the "AFAICT" :-)
| this.owner.env.runtime.add(this.#removeBleTransport(this.#bleTransport)); | ||
| this.#bleTransport = undefined; | ||
| if (this.#bleTransports) { | ||
| for (const ble of this.#bleTransports) { |
lauckhart
force-pushed
the
flexible-server-address
branch
from
1c98d7f
to
77f6516
Compare
Now takes addresses as a list of listening address instead of one each of IPv4, IPv6 and BLE. Adds support for dual-mode (IPv4 + IPv6) sockets.
Adds one dual-mode UDP listener on all addresses if none is configured. Adds one BLE listener if BLE is supported and "ble" option is not set to false.
Throws an error if there are multiple BLE addresses or the HCI address is specified in the listener list (these require lower-level changes I did not make yet).
I want to make similar changes for Mdns at some point but figured I'd see how these go over first.
Includes some additional convenience methods for variable access.