EVALG-77 follow up: Change design to use Last-Value Cache and Read #703
Conversation
|
|
||
| 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.
There was a problem hiding this comment.
Checking info.valid is not necessary when you use read_data(); it already skips invalid data samples.
| ) | ||
|
|
||
| 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.
There was a problem hiding this comment.
You're also using type hints for dds.DataReader, DataWriter, Optional[Coordinate]... It makes the example inconsistent. I would prefer to add type hints for all function arguments and return types, which is a good practice anyway.
|
|
||
| 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
There was a problem hiding this comment.
I don't think we should print that then. Either remove the repr method it and just print ("Dashboard:") only or add a name to the dashboard.
| 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.
| self._metrics_app(), | ||
| self._transit_app(), | ||
| string += f"[INFO] Vehicle {sample.vehicle_vin}" | ||
| if sample.current_route and len(sample.current_route): |
There was a problem hiding this comment.
if sample.current_route:
The and is always true.
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