dufourgilles / knx-ip

KNX IP Gateway protocol implementation
30 stars 9 forks source link

missing heartbeat on monitorBus #6

Closed nyotiemon closed 3 years ago

nyotiemon commented 4 years ago

as in KNX spec:

5.4 Heartbeat monitoring
Host protocols not providing mechanisms for life time check like UDP/IP need a procedure to identify
failure of communication, may it be on the KNX or the tunnelling network. To detect such situations
heartbeat monitoring is defined and shall be implemented by both KNXnet/IP Clients and KNXnet/IP
Servers.
The client shall send a CONNECTIONSTATE_REQUEST frame regularly, i.e. every 60 seconds, to the
server’s control endpoint, which shall respond immediately with a CONNECTIONSTATE_RESPONSE
frame (this counts as heartbeat response).
If the KNXnet/IP Client does not receive the heartbeat response within the CONNECTIONSTATE_-
REQUEST_TIMEOUT (= 10 seconds) or the status of a received heartbeat response signalled any kind of
error condition, the client shall repeat the CONNECTIONSTATE_REQUEST three (3) times and then
terminate the connection by sending a DISCONNECT_REQUEST to the server’s control endpoint.
If the KNXnet/IP Server does not receive a heartbeat request within 120 seconds of the last correctly
received message frame, the server shall terminate the connection by sending a DISCONNECT_-
REQUEST to the client’s control endpoint. The server shall not retrigger the timeout after messages
received with unexpected sequence number.
dufourgilles commented 4 years ago

Do you have the KNX doc ? I can't find them anymore.

nyotiemon commented 4 years ago

KNX spec 03_08_02 Core v01.05.01 AS.pdf The spec is available on KNX official website after login

nyotiemon commented 3 years ago

I saw your new commit (9e23158) and tried the new update. The heartbeat works perfectly by sending connection state request every 60s.

However when the request timed out without response (~10s), there are no retries. Nor there are subsequent request after 60s after the last request. Is this intended?

dufourgilles commented 3 years ago

Actually, I did not want the library to control retry, or reconnect, ... Instead, when an event like dead or timeout occurs, an event gets fired and it is up to your application to decide to retransmit or not.

nyotiemon commented 3 years ago

Ok, I can understand how you design the lib. But somehow KNXTunnelSocket.on("disconnected") didnt get called when connection is lost.


this._knx = new KNXTunnelSocket('0.0.1');
this._knx.bindSocketPortAsync(socketPort);

// setup monitoring callback
this._knx.on(KNXTunnelSocketEvents.indication, (src, dst, npdu) => {
    this._logger.info(`indication ${src.toString()} -> ${dst.toString()}`);
});

// setup reconnect on disconnected
this._knx.on(KNXTunnelSocketEvents.disconnected, async () => {
    this._logger.warn(`disconnected`);
});

// setup error handler
this._knx.on(KNXTunnelSocketEvents.error, (err) => {
    this._logger.error(`error ${err}}`);
})

Tried the disconnect event by unplugging the device, the event never get triggered

nyotiemon commented 3 years ago

Found out that KNXTunnelSocket didn't emit error on KNXClient.on(error), but it emits disconnected.

// KNXTunnelSocket.ts

private _init(): void {
    this._knxClient.on(KNXClient.KNXClientEvents.connected, () => {
        ...
    }).on(KNXClient.KNXClientEvents.error, (err: Error) => {
        // no emit error
    }).on(KNXClient.KNXClientEvents.disconnected, () => {
        // tunnel socket emit disconnected
        this.emit(KNXTunnelSocketEvents.disconnected);
    }).on(KNXClient.KNXClientEvents.discover, this._processDiscoredHost);
}

And while at this, would it be more clear when KNXClient's heartbeat reach max timeout, it will send disconnect request and finished by emitting disconnected event?

// KNXClient.ts

getConnectionStatus(host: string, port: number, channelID?: number): void {
    ...
    this._heartbeatTimer = setTimeout(() => {
        this._heartbeatTimer = null;
        this.emit(KNXClient.KNXClientEvents.error, timeoutError);
        this._heartbeatFailures++;
        if (this._heartbeatFailures >= this.max_HeartbeatFailures) {
            this.emit(KNXClient.KNXClientEvents.error, deadError);

            // either emit disconnected on this line
            this.emit(KNXClient.KNXClientEvents.disconnected, `${host}:${port}`, channelID);

            this.disconnect(host, port, this._channelID); // or inside disconnect method
        }
    }, 1000 * KNX_CONSTANTS.CONNECTIONSTATE_REQUEST_TIMEOUT);
    ...
}

Disconnected event on max heartbeat timeout is a very clear indication that user can/should retry the connection. The error event at the other hand has too much error variant int it. It also will clutter the app if user needs to catch specific error to find out if the socket is disconnected or not

dufourgilles commented 3 years ago

Yes. I will make the change to emit disconnected.