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
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import { Endpoint, EndpointServer } from "@project-chip/matter.js/endpoint";
import { RootRequirements } from "@project-chip/matter.js/endpoint/definitions";
import { Environment, StorageService } from "@project-chip/matter.js/environment";
import { FabricAction } from "@project-chip/matter.js/fabric";
import { Level, Logger, levelFromString } from "@project-chip/matter.js/log";
import { Logger, levelFromString } from "@project-chip/matter.js/log";
import { ServerNode } from "@project-chip/matter.js/node";
import { QrCode } from "@project-chip/matter.js/schema";
import { Time } from "@project-chip/matter.js/time";
Expand Down Expand Up @@ -103,7 +103,7 @@ function executeCommand(scriptParamName: string) {
const logFile = environment.vars.string("logfile.filename");
if (logFile !== undefined) {
Logger.addLogger("filelogger", await createFileLogger(logFile), {
defaultLogLevel: levelFromString(environment.vars.string("logfile.loglevel")) ?? Level.DEBUG,
defaultLogLevel: levelFromString(environment.vars.string("logfile.loglevel", "debug")),
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export class DummyThreadNetworkCommissioningServer extends NetworkCommissioningB
const threadScanResults = [
{
panId: this.endpoint.env.vars.number("ble.thread.panId"),
extendedPanId: BigInt(this.endpoint.env.vars.string("ble.thread.extendedPanId")),
extendedPanId: this.endpoint.env.vars.bigint("ble.thread.extendedPanId"),
networkName: this.endpoint.env.vars.string("ble.thread.networkName"),
channel: this.endpoint.env.vars.number("ble.thread.channel"),
version: 130,
Expand Down
6 changes: 5 additions & 1 deletion packages/matter-node.js/src/net/UdpChannelNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,15 @@ export class UdpChannelNode implements UdpChannel {
listeningAddress,
netInterface,
membershipAddresses,
reuseAddress,
}: UdpChannelOptions) {
const socketOptions: dgram.SocketOptions = { type, reuseAddr: true };
const socketOptions: dgram.SocketOptions = { type: type === "udp" ? "udp6" : type };
if (type === "udp6") {
socketOptions.ipv6Only = true;
}
if (reuseAddress) {
socketOptions.reuseAddr = true;
}
const socket = await createDgramSocket(listeningAddress, listeningPort, socketOptions);
socket.setBroadcast(true);
let netInterfaceZone: string | undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ export abstract class NetworkRuntime {
}
}

abstract operationalPort: number;

protected abstract start(): Promise<void>;

protected abstract stop(): Promise<void>;
Expand Down
155 changes: 144 additions & 11 deletions packages/matter.js/src/behavior/system/network/NetworkServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import { Ble } from "../../../ble/Ble.js";
import { ImplementationError } from "../../../common/MatterError.js";
import { ImplementationError, NotImplementedError } from "../../../common/MatterError.js";
import { Logger } from "../../../log/Logger.js";
import { SubscriptionOptions } from "../../../protocol/interaction/SubscriptionOptions.js";
import { TypeFromPartialBitSchema } from "../../../schema/BitmapSchema.js";
Expand All @@ -27,13 +27,11 @@ export class NetworkServer extends NetworkBehavior {
declare internal: NetworkServer.Internal;

override initialize() {
if (this.state.ble === undefined) {
// TODO make working again when State init gets fixed!
this.state.ble = Ble.enabled;
} else if (this.state.ble && !Ble.enabled) {
logger.warn("Disabling Bluetooth commissioning because BLE support is not installed");
this.state.ble = false;
}
const vars = this.endpoint.env.vars;

this.state.port = vars.number("network.port", this.state.port);

this.state.listen = this.#configureListeners(vars.list("network.listen", this.state.listen));

const discoveryCaps = this.state.discoveryCapabilities;
switch (discoveryCaps.ble) {
Expand Down Expand Up @@ -101,21 +99,156 @@ export class NetworkServer extends NetworkBehavior {
this.internal.runtime.endUncommissionedMode();
}
}

#configureListeners(config: unknown[]) {
const listen = Array<NetworkServer.Address>();
let hasUdp = false;
let hasBle = false;
let disabledBle = false;
for (const addr of config) {
if (typeof addr !== "object") {
throw new ImplementationError("Listen address is not an object");
}

let { protocol, port } = addr as Record<string, any>;
Apollon77 marked this conversation as resolved.
Show resolved Hide resolved
const { address } = addr as Record<string, any>;

if (protocol === undefined) {
protocol = "udp";
}

switch (protocol) {
case "ble":
if (Ble.enabled) {
if (hasBle) {
throw new NotImplementedError("Currently only a single BLE transport is allowed");
} else {
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...

}
listen.push({ protocol, address });
this.state.ble = true;
} else {
disabledBle = true;
}
break;

case "udp":
case "udp4":
case "udp6":
hasUdp = true;
if (port === undefined) {
port = this.state.port;
}
listen.push({ protocol, address, port });
break;

default:
throw new ImplementationError(`Unknown listen protocol "${protocol}"`);
}
}

if (disabledBle) {
logger.warn("Disabling Bluetooth commissioning because BLE support is not installed");
this.state.ble = false;
} else if (this.state.ble !== false && Ble.enabled) {
if (!hasBle) {
listen.push({ protocol: "ble" });
}
this.state.ble = true;
}

if (!hasUdp) {
listen.push({ protocol: "udp", port: this.state.port });
}

return listen;
}
}

export namespace NetworkServer {
/**
* A UDP listening address.
*/
export interface UdpAddress {
protocol: "udp" | "udp4" | "udp6";

/**
* The hostname or IP address. Leave undefined for all addresses, "0.0.0.0" for all IPv4 addresses, and "::"
* for all IPv6 addresses.
*/
address?: string;

/**
* The port to listen on. Defaults to {@link State.port}.
*/
port?: number;
}

/**
* A Bluetooth LE listening address,
*
* TODO - currently only a single BLE transport is supported
*/
export interface BleAddress {
protocol: "ble";

/**
* The HCI ID of the bluetooth adapter.
*
* By default selects the first adapter on the system.
*
* TODO - currently you cannot specify HCI ID here
*/
address?: string;
}

export type Address = BleAddress | UdpAddress;

export class Internal extends NetworkBehavior.Internal {
declare runtime: ServerNetworkRuntime;
}

export class State extends NetworkBehavior.State {
listeningAddressIpv4?: string = undefined;
listeningAddressIpv6?: string = undefined;
ipv4 = true;
/**
* An array of {@link Address} objects configuring the interfaces the server listens on.
*
* 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.

* * `network.listen.address` the hostname, IP address (default all) or HCI ID (default first) to listen on
* * `network.listen.port` the port for UDP listeners (default is 5540)
*
* You may configure multiple listeners using `network.listen.0`, `network.listen.1`, etc. with the same subkeys
* as above.
*
* At least one UDP listener is required. The server will add one if none are present.
*
* If {@link ble} is true, the server will add a BLE listener as well if none are present and Matter.js supports
* BLE on the current platform.
*/
listen = Array<Address>();

/**
* Controls whether BLE is added to the default configuration. If undefined, BLE is enabled if present on the
* system.
*
* Once the server starts this value reflects the current state of BLE for the node.
*/
ble?: boolean = undefined;

/**
* The Matter capabilities the server broadcasts.
*/
discoveryCapabilities: TypeFromPartialBitSchema<typeof DiscoveryCapabilitiesBitmap> = {
onIpNetwork: true,
};

/**
* Time intervales for subscription configuration.
*/
subscriptionOptions?: SubscriptionOptions = undefined;
}
}
Loading
Loading