feat(python): add InputAligner on humble#281
Conversation
* feat(python): add InputAligner on humble * fix(python): use ROS time for InputAligner timestamps * fix(python): avoid rclpy.clock_type dependency on Humble * test(python): expand InputAligner parity coverage * test(python): add remaining InputAligner parity cases * style(python): polish InputAligner time helpers
|
@ahcorde Hi Alejandro, could you please take a look at this PR when you have a moment? I’m not sure whether it would be better to bring InputAligner back to Humble. If that makes sense, I can continue working on the C++ version. |
ahcorde
left a comment
There was a problem hiding this comment.
Do you mind to target rolling ? This change is backportable
| from rclpy.qos import QoSProfile | ||
| from rclpy.time import Time | ||
|
|
||
| from .input_aligner import InputAligner, QueueStatus |
There was a problem hiding this comment.
| from .input_aligner import InputAligner, QueueStatus | |
| from messaege_filters.input_aligner import InputAligner, QueueStatus |
? Absolute imports recommended by PEP-8 over relative importls. This option is acceptible, but maybe it is better to use absoleute, as it is not that complex.
| return QueueStatus(self.active, len(self.events), self.msgs_processed, self.msgs_dropped) | ||
|
|
||
|
|
||
| class InputAligner: |
There was a problem hiding this comment.
Should not it be inheriting from SimpleFilter? It seems like a successor to that but is not. I understand that it will prevent it from being imported in the __init__.py. But the SimpleFilter is going to be removed from there in this task. After that, this filter may be easely imported there.
I may add the inheritance later, but not now? :)
|
|
||
|
|
||
| class InputAligner: | ||
| def __init__(self, timeout, *filters): |
There was a problem hiding this comment.
Maybe add type hints?
| def __init__(self, timeout, *filters): | |
| def __init__( | |
| self, | |
| timeout: int, | |
| *filters: list[SimpleFilter], | |
| ): |
| cb(*(msg + args)) | ||
|
|
||
|
|
||
| def _ros_zero_time(): |
There was a problem hiding this comment.
| def _ros_zero_time(): | |
| def _ros_zero_time() -> Time: |
?
| def _ros_max_time(): | ||
| zero = _ros_zero_time() | ||
| return Time(nanoseconds=9223372036854775807, clock_type=zero.clock_type) |
There was a problem hiding this comment.
| def _ros_max_time(): | |
| zero = _ros_zero_time() | |
| return Time(nanoseconds=9223372036854775807, clock_type=zero.clock_type) | |
| def _ros_max_time() -> Time: | |
| return Time( | |
| nanoseconds=9223372036854775807, | |
| clock_type=_ros_zero_time().clock_type, | |
| ) |
?
|
|
||
| class _EventQueue: | ||
| def __init__(self): | ||
| self.events = [] |
There was a problem hiding this comment.
Maybe use Queue.queue. As one of the advantages, it is threadsafe.
| self.event_queues = [] | ||
| self.input_connections = [] | ||
| self.signals = [] | ||
| self.dispatch_timer = None |
There was a problem hiding this comment.
Maybe add type hints for all these fields as well? May be useful for list fields for sure as the declaration does not provide any information on the stored data itself.
| def disconnectAll(self): | ||
| self.input_connections = [] | ||
|
|
||
| def registerCallback(self, index, cb, *args): |
There was a problem hiding this comment.
| def registerCallback(self, index, cb, *args): | |
| def registerCallback( | |
| self, | |
| index: int, | |
| callback: tp.Callable[*actual acceptible arguments and returns list*], | |
| *args, | |
| **kwargs, | |
| ): |
| def pop_first(self): | ||
| self.events.pop(0) | ||
| self.msgs_processed += 1 |
There was a problem hiding this comment.
Shouldn't pop type functions return the oped value?
| def pop_first(self): | |
| self.events.pop(0) | |
| self.msgs_processed += 1 | |
| def pop_first(self) -> MsgT: | |
| self.msgs_processed += 1 | |
| return self.events.pop(0) |
?
| self.msgs_processed = 0 | ||
| self.msgs_dropped = 0 | ||
|
|
||
| def first_timestamp(self): |
There was a problem hiding this comment.
get_first_timestamp? activate? name suggests a property that returns timestamp, but actually it is a method that changes the state of a class instance.
There was a problem hiding this comment.
I tried to keep consistency between py version and
. If you could help to make sureget_first_timestamp looks better, I can take it.
|
Thanks for reviewing this PR. I’m planning to contribute the same change to Rolling once this gets merged. However, I ran into an issue while testing in a Would you happen to know the correct fix or the intended approach here? If I can get Rolling building properly on my side, I’d be happy to validate everything there as well before sending a follow-up patch. Thanks again for your help. |
| with self.lock: | ||
| if not any(queue.events for queue in self.event_queues): | ||
| return | ||
| input_available = True | ||
| while input_available: | ||
| input_available = self._dispatch_first_message() |
There was a problem hiding this comment.
| with self.lock: | |
| if not any(queue.events for queue in self.event_queues): | |
| return | |
| input_available = True | |
| while input_available: | |
| input_available = self._dispatch_first_message() | |
| with self.lock: | |
| if all(queue.events for queue in self.event_queues): | |
| input_available = True | |
| while input_available: | |
| input_available = self._dispatch_first_message() | |
should it be all though?
| timestamps = [queue.first_timestamp() for queue in self.event_queues] | ||
| idx = min(range(len(timestamps)), key=lambda i: timestamps[i].nanoseconds) | ||
| queue = self.event_queues[idx] | ||
| if queue.events: |
There was a problem hiding this comment.
It seems to me that with the previous logic based on not any(queue.events for queue in self.event_queues) this condition will always be True. And with the suggested in my previous comment as well. At least there is another place, where this method is invoked. If you switch to any(queue.events for queue in self.event_queues), it could be False. But it seems to me that these two functions should be rearranged so you will not do so many loops over the lists and queues to dispatch a single message. I'll try to think on a better solution and come back as soon as I have one. Soory for not having one already. It may require some time.
There was a problem hiding this comment.
There is a case:
- [
dispatchMessages] only part of the queue are empty. - [
_dispatch_first_message] queue.events could beFalse.
That's why we might not switch to all.
By "not do so many loops", if I understand well, it's because we need to search for the "latest timestamp" by loop, even there is only single message in a single queue. A solution to that might be to switch to priority queue.
|
A bit nitpicky comment, but shouldn't a PR be linked to an issue?
I'll try to take a look at the upd Failed to find some time to take a look at the |
I agree to the idea of targeting this to |
|
Here is the issue: #282 |
|
Hello @dvorak0 ! I've managed to take a look at the building with rolling issue. I've started with the So I think you may do the same. Just rebase your branch on You may cherry-pick your commits on top of the |
I got error with The log shows: @EsipovPA I'm guess there is issue regarding CMake exports. But I'm not sure. |
|
@dvorak0 In my case, I had a pretty old version of a rolling branch of all the ROS2 code, so I removed build and install folders, redownloaded all the sources and built everything from scratch with
But I've needed the update anyways. Maybe for you it will only take the removing of |
@EsipovPA Then I get #283 . What do you think? I believe the What do you think of the "docker run" compilation? By using that, we could keep in the same page of dev envs. |
|
@dvorak0 I'm not sure about the In my case, I've successfully built a package and completed almost all the tests. Here is my output The build The test Do you use the same build, or do you download a docker image and build using pre-built packages? |
@EsipovPA I downloaded the image and thus used the pre-built packages from the image. It might explain. |
|
@dvorak0 Well, anyway, since you've opened the new PR, it should run the build with the latest |
Summary
InputAlignerimplementation on top of thehumblebranchtest_input_aligner.cppcoverageDetails
This PR targets
ros2/message_filters:humblefromUniflexAI/message_filters:humble.The Python implementation follows the C++ InputAligner semantics referenced from the upstream
sa-input_alignerwork.Test coverage
The Python test file covers the same 8 cases as the C++
test_input_aligner.cpp:Validation
Validated on a Humble environment with all 8 Python InputAligner tests passing.