Execute disonnected_callback only when websocket is disconnected

[Pietfried]

Without this change, the disconnected_callback was executed also on reconnect attempts when the websocket is already disconnected. This leads to the situation that the time_disconnected of ChargePoint resets on reconnect attempts and therefore the handling of the OfflineThreshold is incorrect. This change only calls disconnected_callback if websocket is not already disconnected.

Fixes TC_B_57_CS

[a-w50]

Is it possible to have common logic for the different websocket implementations (plain, tls, tpm ...)? Right now this looks a bit like code duplication.

[hikinggrass]

Is it possible to have common logic for the different websocket implementations (plain, tls, tpm ...)? Right now this looks a bit like code duplication.

The current state is intentional as we're slowly moving over from websocket++ (used in _plain and _tls) to libwebsockets (only used for _tpm at the moment).
The tpm implementation was the proving ground for testing out libwebsockets, next up there will be a interface refactoring to get rid of a leaky abstraction (that unfortunately leaked out some websocket++ implementation details). Once this is done we'll implement plain and TLS websocket handling with libwebsockets as well. This will lead to having two independent, compile time selectable implementations of the websocket backend. And once the libwebsockets based implementation is sufficiently tested (and either equal to or better than the websocket++ based implementation) we will completely switch over to libwebsockets and remove websocket++ as a dependency.

Work on this will be tracked in #387

[a-w50]

The current state is intentional as we're slowly moving over from websocket++ (used in _plain and _tls) to libwebsockets (only used for _tpm at the moment). The tpm implementation was the proving ground for testing out libwebsockets, next up there will be a interface refactoring to get rid of a leaky abstraction (that unfortunately leaked out some websocket++ implementation details). Once this is done we'll implement plain and TLS websocket handling with libwebsockets as well. This will lead to having two independent, compile time selectable implementations of the websocket backend. And once the libwebsockets based implementation is sufficiently tested (and either equal to or better than the websocket++ based implementation) we will completely switch over to libwebsockets and remove websocket++ as a dependency.

Work on this will be tracked in #387

I guess I didn't express myself well, it is not about websocket details leaking into ocpp logic, it is more the other way around. This PR does now 3 times the following:

    if (this->m_is_connected) {
        this->disconnected_callback();
    }

Of course it is debatable, but I would argue that this should be common logic or code. One could do the m_is_connected check inside a common disconnect handler, which then will eventually call the disconnect hook.

[Pietfried]

The current state is intentional as we're slowly moving over from websocket++ (used in _plain and _tls) to libwebsockets (only used for _tpm at the moment). The tpm implementation was the proving ground for testing out libwebsockets, next up there will be a interface refactoring to get rid of a leaky abstraction (that unfortunately leaked out some websocket++ implementation details). Once this is done we'll implement plain and TLS websocket handling with libwebsockets as well. This will lead to having two independent, compile time selectable implementations of the websocket backend. And once the libwebsockets based implementation is sufficiently tested (and either equal to or better than the websocket++ based implementation) we will completely switch over to libwebsockets and remove websocket++ as a dependency.
Work on this will be tracked in #387

I guess I didn't express myself well, it is not about websocket details leaking into ocpp logic, it is more the other way around. This PR does now 3 times the following:

    if (this->m_is_connected) {
        this->disconnected_callback();
    }

Of course it is debatable, but I would argue that this should be common logic or code. One could do the m_is_connected check inside a common disconnect handler, which then will eventually call the disconnect hook.

There is indeed quite some duplicated code in the websocket plain and tls implementations. The refactoring of the websocketpp implementation has so far been postponed because other priorities were set and the switch to libwebsockets was planned anyway. Libwebsockets is to replace websocketpp, so the effort of refactoring the websocketpp impl was avoided so far.

[hikinggrass]

I guess I didn't express myself well, it is not about websocket details leaking into ocpp logic, it is more the other way around. This PR does now 3 times the following:

    if (this->m_is_connected) {
        this->disconnected_callback();
    }

Of course it is debatable, but I would argue that this should be common logic or code. One could do the m_is_connected check inside a common disconnect handler, which then will eventually call the disconnect hook.

You're absolutely right, and I think we all agree that this should be refactored soon. I justed wanted to give some context that a proper refactoring is already underway which addresses a few more issues as well 🙂 so any time and effort spent on improving this part of the lib will be more wisely spent there once the PRs are opened