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

Implement VeQItemMqtt and VeQItemMqttProducer #2

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

chriadam
Copy link

@chriadam chriadam commented Feb 1, 2023

No description provided.

@chriadam chriadam force-pushed the chriadam-qtmqtt-veqitemmqtt branch from 1e3165a to 6d4a655 Compare March 24, 2023 06:37
@chriadam chriadam force-pushed the chriadam-qtmqtt-veqitemmqtt branch 2 times, most recently from c5d1a49 to 0661ff7 Compare April 20, 2023 05:30
@chriadam chriadam force-pushed the chriadam-qtmqtt-veqitemmqtt branch from 395fd9b to 77167ce Compare May 9, 2023 09:55
@chriadam chriadam force-pushed the chriadam-qtmqtt-veqitemmqtt branch from 60b517c to d9d618c Compare June 7, 2023 07:37
@chriadam chriadam force-pushed the chriadam-qtmqtt-veqitemmqtt branch 2 times, most recently from b69bd3d to 9f5651e Compare June 28, 2023 12:02
@blammit blammit force-pushed the chriadam-qtmqtt-veqitemmqtt branch 2 times, most recently from 811eee3 to 3d96108 Compare August 1, 2023 00:08
Copy link

@wiebeytec wiebeytec left a comment

Choose a reason for hiding this comment

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

From the look of it, mMissedHeartbeats is not used for anything?

The heartbeat algorithm described in the 'Designing a Victron UI' doc uses this counter to have three states: real-time, 'orange icon' (currently has no name) and not-real-time. So, I think the HeartbeatState needs another state. Perhaps HeartbeatMissed?

In the document we decided on 5 seconds as a timer interval, instead of 3500. Having experience monitoring the servers, we cannot always guarantee a 500 ms delivery.

} else if (topicName.compare(heartbeatTopic, Qt::CaseInsensitive) == 0) {
// (re)start our heartbeat timer.
mHeartBeatTimer->start();
mMissedHeartbeats = 0;
Copy link

Choose a reason for hiding this comment

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

Is mMissedHeartbeats needed? It seems the value is not actually used anywhere.

Choose a reason for hiding this comment

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

Also see my comment. It can/should be used, to have three heartbeat states.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, updated.

Just double checking: is it correct that it should transition to HeartbeatMissed when count = 2, and to HeartbeatInactive when count = 3? That is my reading of the doc, but perhaps I misunderstand and it should be HeartbeatMissed if count is 1 or 2, and HeartbeatInactive if count is 3?

Copy link
Author

Choose a reason for hiding this comment

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

(Also updated interval to 5s as per doc and your comment - thanks)

Choose a reason for hiding this comment

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

@chriadam The values 2 and 3 are perhaps too high; they came from the algorithm for pre-heartbeat Venus versions. In that, because the response depends on publishing R/<portalid here>/system/0/Connected first, it's given one cycle extra to get a response. I think maybe we need to learn from the field whether '1 and 2' or '2 and 3' are good values.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, then let's leave as-is for now, and revisit later if we find a need to do so.

@chriadam chriadam force-pushed the chriadam-qtmqtt-veqitemmqtt branch from 9ab8023 to afed9b0 Compare August 28, 2023 05:18
@chriadam chriadam force-pushed the chriadam-qtmqtt-veqitemmqtt branch from fa53a6e to dc2dff1 Compare November 13, 2023 03:54
chriadam and others added 26 commits January 8, 2024 13:03
- generate prefixed clientId to allow filtering server logs by
  specific client prefixes
- implement exponential backoff in reconnection intervals
- provide (re)connect hook to allow client to specify credentials
- fix protocol string
- use specific bootstrap subscription instead of "#" wildcard
- use QueuedConnection lambda invocation in continueConnect()
  to ensure that any previous deleteLater() is serviced before
  connectToHost() is performed, to avoid possible race condition.
Allow the client to provide the portalId, for VRM connections.
Don't transition to Ready state unless we have received a message.
Also fix a bug in the clientId generation code.
Also properly support clients calling open() from different thread
by ensuring that we construct our child objects in the appropriate
thread context, via QueuedConnection call to inner function.
Qt For WebAssembly doesn't process the events in a timely manner
(unless the event queue is kicked by a timer event), so abandon
the asynchronous queue.  This may be fixed in a future version of
Qt, at which point in time it may be beneficial to revert this
commit, if stutters/UI stalls are noticed when receiving large
batches of MQTT notifications.
The client now has the option whether to read the last-valid value
or the most-recent (even if invalid) value.

To support this, we need to ensure that we produce a
specified-as-invalid value whenever connection to the backend is
lost, or when the backend tells us that a specific device has been
disconnected (via a null payload message for a topic).
Qt MQTT already emits the appropriate disconnected signal and
closes the websocket when it detects that the websocket has
disconnected, so the previous manual handler caused the signals
to be emitted twice.

However, in non-websocket case, we also need to handle the case
where the initial connection fails (and manually trigger our
onDisconnected() handler, to perform reconnect etc).
We detect this by handling error change and state change from
the MQTT client.

Finally, fix the reconnect count (as it was previously reset in
a queued connection).
Every value will be updated automatically due to the keepalive
request on reconnect.

Also add a note about the problem with Qt not exposing the retain
flag value for received messages.
Previously, we transitioned to Ready state as soon as we received
the first message (since that meant that communication with the
broker had been established).

Now we transition to Initializing state once we receive the first
message, and only transition to Ready state once we have received
all of the initial messages triggered by the first KeepAlive.

This ensures that data is in a coherent state when Ready.

Also, construct timers as children of the producer, to ensure that
timer events are serviced on the producer's thread.
- ensure that we send an empty keepalive message right after
  subscribing to R/portalId/#
- only then should the periodic 30s keepalive timer be started
- when the periodic keepalive timer triggers, send a keepalive msg
  with payload: { "keepalive-options" : ["suppress-republish"] }
- don't trigger spurious keepalive when transitioning to the
  Initializing state (i.e. while waiting for initial messages)

Also increase the Initializing fallback timeout timer interval
to ten seconds, and the Initializing inter-message timeout timer
interval to one second, in case there is a lot of initial traffic
from the broker.
Disable the "ignore retained messages" feature for VRM connections
until VRM switches over to FlashMQ.

Also disable the "suppress-republish" keepalive for VRM
connections until VRM switches over to FlashMQ.
87fded3 added polling capabilities
for VeQItem implementations over D-Bus, via getValue(true).
This commit adds the implementation for MQTT connections.
Safari will disconnect the websocket when the tab is idle, in
order to save power etc.  When this occurs, we need the device
to recognise this case and close() the socket (client side),
so that the QMqttClient detects the new IO device state, and
transitions properly to Disconnected state.

Then, the UI code can detect the state transition, and rebuild
the data connection and the UI appropriately.
The VRM broker will be updated to ensure that this isn't a problem
in future once more clients connect to VRM.

FlashMQ on the CerboGX doesn't emit retained messages (except for
the serial and keepalive topics) so it doesn't need special
handling there.
Wait no longer than 4s to receive all "initial" messages from the
broker, before transitioning to Ready state.
This is a convenience property for checking whether the property
value is undefined.
See QTBUG-118820 for the QtMqtt bug.
See halfgaar/FlashMQ@f43245e
for the related FlashMQ fix.
FlashMQ broker now sends a "full_publish_completed" message once
it has sent all of the initial-state messages.

So, there is no longer any need to use a timing heuristic to
determine when all initial-state messages have been received.
@blammit blammit force-pushed the chriadam-qtmqtt-veqitemmqtt branch from 7116477 to e731a43 Compare January 8, 2024 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants