Implementation of the Keep Alive thread in the transport service. #281

LePailleurThibault · 2024-11-22T09:52:05Z

No description provided.

hound · 2024-11-22T13:19:42Z

python_transport/wirepas_gateway/keep_alive_service/__init__.py

+#
+# See file LICENSE for full license details.
+#
+from .keep_alive_service import *


'.keep_alive_service.*' imported but unused

@LePailleurThibault Is it a valid comment from hound?

Well, i don't know how this comment is generated and imports in a init file are never really "used".
However, for example, in the transport service there is this line
from wirepas_gateway.keep_alive_service import KeepAliveServiceThread.
The init.py is required but maybe not this line

hound · 2024-11-22T13:19:42Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+
+        Args:
+            sink_manager: The sink manager to send sinks the keep alive messages.
+            mqtt_wrapper: The mqtt wrapper to get access to queue level of the mqtt broker.


line too long (91 > 79 characters)

hound · 2024-11-22T13:19:42Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+            )
+        if self.keep_alive_interval_s is not None:
+            buffer += KeepAliveMessage.encode_tlv_item(
+                KeepAliveType.KEEP_ALIVE_INTERVAL_TYPE, 2, self.keep_alive_interval_s, "H",


line too long (91 > 79 characters)

hound · 2024-11-22T13:19:42Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+            Bits 1-7: Reserved for future use or other status indicators
+        rtc_timestamp_ms: Unix epoch timestamp in ms (milliseconds since January 1, 1970).
+        timezone_offset_mn: Time zone offset from UTC in minutes (-840 to +720).
+        keep_alive_interval_s: Interval in seconds until the next keepalive message is expected.


line too long (96 > 79 characters)

hound · 2024-11-22T13:19:43Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+    Attributes:
+        version: The version number for the keep-alive message.
+        gateway_status: The running status of the gateway.
+            Bit 0: Backhaul (MQTT broker) Connectivity (0 = Disconnected, 1 = Connected)


line too long (88 > 79 characters)

sgunes-wirepas · 2025-01-07T16:04:46Z

Maybe a more general question, but why is the timezone offset needed in the keepalive messages? In locations with daylight saving time, the offset from UTC would change throughout the year.

LePailleurThibault · 2025-01-09T12:22:44Z

These messages will be used for example in the dlms app where the application might update the connected meter local time with the UTC time + timezone offset.
But, that's a good point, I will change the timezone offset input to a timezone name, so that the module will handle daylight saving time changes.

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

GwendalRaoul · 2025-01-09T13:47:06Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+KEEP_ALIVE_DST_EP = 67
+
+# Timeouts and periods used for the keep alive service.
+WM_MSG_RETRY_PERIOD_S = 1


Is 1s enough ?

Well if a message can't be sent to a sink 3 times in a row in ~2s, there are definitely issue with this sink. So not sending a keep alive message for that sink wouldn't be that bad.

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

GwendalRaoul · 2025-01-09T13:48:44Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+                src_ep=KEEP_ALIVE_SRC_EP,
+                dst_ep=KEEP_ALIVE_DST_EP,
+                qos=0,
+                initial_time=0,


If I recall right, it is already the default value, so could be skipped

This argument is required by the method. I don't think there is any default value for the initial_time.

GwendalRaoul · 2025-01-09T13:51:57Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+        """ Main loop that send periodically keep alive message to the network. """
+        while True:
+            # Put a timer so that the message are periodic with a good precision
+            start_timer = time()


Shouldn't time.monotonic() be used instead, to avoid issue if clock is changed during the process?

Good point, monotonic would be much safer. How about the nodes, can it be a problem there if the keepalive message has an older timestamp than the previous one?

Yes, I will use a monotonic timer here. The gateway is supposed to be connected to a NTP server when running the keep alive service and the keep alive messages contain the UTC timestamp. This issue will never happen with this timestamp. Now if the application wants to use local time (based on UTC + timezone offset), the application itself should handle the consequence of DST.

I was thinking more about the following (very unlikely) scenario:

NTP server(s) becomes unreachable during runtime, but not MQTT

Host clock drifts forward

NTP is reconnected and clock is updated

New keepalive message has an older timestamp than the previous one

Time should not be included in the keep alive packet if the NTP server time is unavailable. However, at this moment, we do not have any NTP service-like API for this kind of issue. So there is no way of knowing if this time is valid without implementing a NTP server nearby.
But at the moment, when using the time of this service, the gateway is supposed to be synchronized with a NTP server.
And if it the issue to happen, the time will not be updated in the current applications if the clock difference is too big between 2 packets.

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

hound · 2025-01-10T16:58:17Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+            for sink_id in current_sinks:
+                self.send_keep_alive_msg_to_sink(self.sink_manager.get_sink(sink_id))
+
+            self.wait_for_next_keep_alive_message_iteration(self.keep_alive_interval_s, start_timer)


line too long (100 > 79 characters)

hound · 2025-01-10T16:58:17Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+
+            if not current_sinks:
+                logging.error("No sinks are detected!")
+                self.wait_for_next_keep_alive_message_iteration(self.keep_alive_interval_s)


line too long (91 > 79 characters)

hound · 2025-01-10T16:58:17Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+
+        return True
+
+    def wait_for_next_keep_alive_message_iteration(self, time_to_wait, start_timer=None):


line too long (89 > 79 characters)

hound · 2025-01-10T16:58:17Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+            )
+            if res != wmm.GatewayResultCode.GW_RES_OK and retries_left > 0:
+                sleep(WM_MSG_RETRY_PERIOD_S)
+                logging.debug("Retry sending the keep alive message that couldn't be sent to %s sink: %s. ",


line too long (108 > 79 characters)

hound · 2025-01-10T16:58:17Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+                The interval in seconds between keep-alive messages.
+            keep_alive_timezone_name (str): Default to "Etc/UTC".
+                Time zone name used to set the timezone offset in the keep alive message.
+                Check https://en.wikipedia.org/wiki/List_of_tz_database_time_zones#List


line too long (87 > 79 characters)

hound · 2025-01-10T16:58:17Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+            keep_alive_interval_s (int): Default to 300 seconds.
+                The interval in seconds between keep-alive messages.
+            keep_alive_timezone_name (str): Default to "Etc/UTC".
+                Time zone name used to set the timezone offset in the keep alive message.


line too long (89 > 79 characters)

hound · 2025-01-10T16:58:17Z

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py

+
+class KeepAliveType(IntEnum):
+    """Keep alive fields TLV type enumerate."""
+    VERSION_TYPE = 0x01


Black would make changes.

LePailleurThibault requested a review from GwendalRaoul November 22, 2024 09:52

LePailleurThibault force-pushed the keep_alive_service branch from 9adb176 to 803ac59 Compare November 22, 2024 13:19

hound bot reviewed Nov 22, 2024

View reviewed changes

GwendalRaoul requested a review from sgunes-wirepas January 7, 2025 08:12

GwendalRaoul reviewed Jan 9, 2025

View reviewed changes

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py Outdated Show resolved Hide resolved

GwendalRaoul reviewed Jan 9, 2025

View reviewed changes

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py Outdated Show resolved Hide resolved

GwendalRaoul reviewed Jan 9, 2025

View reviewed changes

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py Outdated Show resolved Hide resolved

GwendalRaoul reviewed Jan 9, 2025

View reviewed changes

python_transport/wirepas_gateway/keep_alive_service/keep_alive_service.py Outdated Show resolved Hide resolved

hound bot reviewed Jan 10, 2025

View reviewed changes

LePailleurThibault force-pushed the keep_alive_service branch from d52d583 to f39881b Compare January 10, 2025 17:03

LePailleurThibault added 2 commits April 14, 2025 14:34

Implementation of the Keep Alive thread in the transport service.

f6f8b69

Fix issues in the keep alive service based on PR#281.

1f77443

LePailleurThibault force-pushed the keep_alive_service branch from 6880b52 to 84b3f6d Compare April 14, 2025 12:34

Update the keep alive service to the latest version

4e343c2

LePailleurThibault force-pushed the keep_alive_service branch from 84b3f6d to 4e343c2 Compare April 14, 2025 13:16


		return True

		def wait_for_next_keep_alive_message_iteration(self, time_to_wait, start_timer=None):

Implementation of the Keep Alive thread in the transport service. #281

Are you sure you want to change the base?

Implementation of the Keep Alive thread in the transport service. #281

Uh oh!

Conversation

LePailleurThibault commented Nov 22, 2024

Uh oh!

hound bot Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hound bot Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

hound bot Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

hound bot Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

hound bot Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

sgunes-wirepas commented Jan 7, 2025

Uh oh!

LePailleurThibault commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hound bot Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

LePailleurThibault commented Jan 9, 2025 •

edited

Loading