EVALG-77 follow up: Change design to use Last-Value Cache and Read #703
Conversation
| display_thread.join() | ||
|
|
||
| def _new_position_string(self, condition): | ||
| string = str() |
There was a problem hiding this comment.
| string = str() | |
| string = "" |
|
|
||
| def _new_position_string(self, condition): | ||
| string = str() | ||
| for sample, info in self._transit_reader.select().condition(condition).read(): |
There was a problem hiding this comment.
You're not using the info:
| for sample, info in self._transit_reader.select().condition(condition).read(): | |
| for data in self._transit_reader.select().condition(condition).read_data(): |
There was a problem hiding this comment.
I am using info in line 91.
|
|
||
| string += f"[INFO] Vehicle {sample.vehicle_vin}" | ||
| if sample.current_route and len(sample.current_route): | ||
| string += f" is enroute to {sample.current_route[-1]} from {sample.current_position}" |
There was a problem hiding this comment.
| string += f" is enroute to {sample.current_route[-1]} from {sample.current_position}" | |
| string += f" is en route to {sample.current_route[-1]} from {sample.current_position}" |
| self._display_app(), | ||
| self._metrics_app(), | ||
| self._transit_app(), | ||
| new_position_condition = dds.ReadCondition( |
There was a problem hiding this comment.
The code needs comments for non-trivial logic that we don't explain in the guide. For example, here we need to explain that the position is updated live, using this condition to notify the handler when new updates for the position topics are received, while the dashboard is printed periodically.
| ) | ||
|
|
||
| def _dashboard_data(self): | ||
| data: typing.Dict[dds.InstanceHandle, DashboardItem] = {} |
There was a problem hiding this comment.
If you use typing for a variable here, you should also use it for the parameters and return types.
Also, prefer data: Dict[..., ...], and from typing import Dict, ....
There was a problem hiding this comment.
Some type hints can be easily inferred, but creating an empty dictionary is not one of these scenarios. I'm limiting hinting to these scenarios instead of being explicit in every scenario.
| string += "\n" | ||
| return string | ||
|
|
||
| def _dashboard_string(self): |
There was a problem hiding this comment.
Use verbs for methods e.g.: _create_dashboard_string or _render_dashboard
| @@ -45,71 +38,110 @@ def __init__( | |||
| ): | |||
There was a problem hiding this comment.
why is dds.DataReader between quotes?
Should be:
def __init__(
self,
metrics_reader: dds.DataReader,
transit_reader: dds.DataReader,
) -> None:
|
|
||
| def __repr__(self): | ||
| return f"Dashboard({self._metrics_reader=}, {self._transit_reader=}, {self._dashboard_data=})" | ||
| return f"Dashboard({self._metrics_reader=}, {self._transit_reader=})" |
There was a problem hiding this comment.
Does the string representation of the DataReader produce anything useful?
Can you show in the PR an example output of a running dashboard?
There was a problem hiding this comment.
I'm not sure if I'd call it useful, as it prints the pointer-like repr of the entities.
❯ python3 -m subscriber
Running dashboard: dashboard=Dashboard(self._metrics_reader=<rti.connextdds.DataReader object at 0x7f360e5e2bb0>, self._transit_reader=<rti.connextdds.DataReader object at 0x7f360e5324f0>)
[[ DASHBOARD: 2024-09-30 10:02:21.532963 ]]
Online vehicles: 0
Offline vehicles: 0
[INFO] Vehicle PALN6ZXN71WG318XU is enroute to Coord(lat=-46.10992285726171, lon=-23.36607730023902) from Coord(lat=-17.29690932734892, lon=36.59023701656847)
# ...
[[ DASHBOARD: 2024-09-30 10:02:26.533522 ]]
Online vehicles: 1
Vehicle PALN6ZXN71WG318XU:
Fuel updates: 6
Last known destination: Coord(lat=-46.10992285726171, lon=-23.36607730023902)
Last known fuel level: 81.3764490923471
Offline vehicles: 0
# ...
[INFO] Vehicle PALN6ZXN71WG318XU has arrived at its destination in Coord(lat=29.106641859065363, lon=41.93699410392373)
[INFO] Vehicle PALN6ZXN71WG318XU is enroute to Coord(lat=9.017138169227357, lon=36.99741430219118) from Coord(lat=29.106641859065363, lon=41.93699410392373)
[INFO] Vehicle PALN6ZXN71WG318XU is enroute to Coord(lat=9.017138169227357, lon=36.99741430219118) from Coord(lat=-28.712438759150604, lon=32.36664575065243)
[[ DASHBOARD: 2024-09-30 10:02:46.534679 ]]
Online vehicles: 0
Offline vehicles: 1
Vehicle PALN6ZXN71WG318XU
| while not display_handler_sentinel.is_set(): | ||
| with display_handler_condition: | ||
| display_handler_condition.wait(5) | ||
| dashboard_condition.trigger_value = True |
There was a problem hiding this comment.
can you explain why you use a GuardCondition instead of just calling print(self._dashboard_string()) here?
There was a problem hiding this comment.
If I did not use a Guard Condition and instead printed after every dispatch, I would always get the dashboard printed whenever the waitset is dispatched.
Using a guard condition, I can make it so that the Guard Condition is printed consistely over time.
|
|
||
| dashboard_condition.set_handler(dashboard_handler) | ||
|
|
||
| display_handler_sentinel = threading.Event() |
There was a problem hiding this comment.
I'm not sure the event and the condition are necessary. You can just use sleep and rely on the KeyboardException to exit the sleep and the thread. I don't think you need to synchronize it by hand with an Event.
Summary
This PR changes the example to follow a Last-Value Cache pattern on the subscriber. This removes the need for asynchronous code and focuses on polling.
Additionally, a few other changes have been made as left-over TODO from previous PR, #695.
Details and comments
Checks