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

Make server ports non-reusable and NetworkServer config more flexible #721

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lauckhart
Copy link
Collaborator

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.

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.
@lauckhart
Copy link
Collaborator Author

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 lauckhart changed the title Change NetworkServer address configuration Make server ports non-reusable and make NetworkServer config more flexible Mar 8, 2024
@lauckhart lauckhart changed the title Make server ports non-reusable and make NetworkServer config more flexible Make server ports non-reusable and NetworkServer config more flexible Mar 8, 2024
Copy link
Collaborator

@Apollon77 Apollon77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments. I think we should include hciId support directly because thats needed if you want to use ble in many cases.

We can also change ble.enable and ble.hciId that I used in example full to the new style network.* stuff and such, no problem

hasBle = true;
}
if (address !== undefined) {
throw new NotImplementedError("Currently you may not specify HCI ID for BLE transport");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: why not define hci id as "address" in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

*
* 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a single BLE is making sense. forbid multiple

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLE is by definition commisioning only

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is so we can identify which ones to remove

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only one?!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants