feat(python): add python implementation of InputAligner #283
feat(python): add python implementation of InputAligner #283dvorak0 wants to merge 24 commits intoros2:rollingfrom
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
|
@EsipovPA Here is the rolling version. Would you help again? Thanks. |
ahcorde
left a comment
There was a problem hiding this comment.
- Tie-break crash — input_aligner.py:108 puts (timestamp, msg) into a PriorityQueue. Equal timestamps fall through to comparing messages → TypeError. Add a sequence counter: (ts, seq,
msg). The C++ version (a std::multiset keyed on timestamp) sidesteps this. - disconnectAll() doesn't disconnect — input_aligner.py:81. It clears only the local list; upstream SimpleFilter callbacks stay live. Re-calling connectInput with fewer inputs →
IndexError on the next upstream message. SimpleFilter (init.py:54) has no unregister API — that needs to be added. - Race in connectInput — input_aligner.py:72-79 replaces event_queues/signals without the lock while add can still fire.
|
@ahcorde I've fixed the issues, and added two more tests coordinating to issue 1 & 2. |
| def __init__(self): | ||
| self.callbacks = {} | ||
|
|
||
| def registerCallback(self, cb, *args): |
There was a problem hiding this comment.
| def registerCallback(self, cb, *args): | |
| def registerCallback( | |
| self, | |
| callback: tp.Callable[ | |
| [ | |
| expected, | |
| argument, | |
| types | |
| ], | |
| expected_return_type | |
| ], | |
| *args | |
| ) -> int: |
As the typing is already imported, why not use it to the full extend? Is the return type an int?
|
|
||
| class _Signal: | ||
| def __init__(self): | ||
| self.callbacks = {} |
There was a problem hiding this comment.
| self.callbacks = {} | |
| self.callbacks: dict[key_type, value_type] = {} |
It seems to me that the value_type is something like tp.Callable[[...], ...]
| def registerCallback( | ||
| self, | ||
| index: int, | ||
| callback: tp.Callable[..., tp.Any], |
There was a problem hiding this comment.
IMHO, this type hint is not really informative. It is the same as just
| callback: tp.Callable[..., tp.Any], | |
| callback: tp.Callable, |
But it does not privide info about the callback signature. Myabe extend this type hint?
There was a problem hiding this comment.
I agreed the info is too loose. How can I do better when they are callbacks, which means I don't know about it much. What do you recommend to do.
There was a problem hiding this comment.
I'll need to take a look into the source code for that. I'll be back with an answer.
Description
Fixes #282
Is this user-facing behavior change?
Did you use Generative AI?
Codex
Additional Information
Summary
InputAlignerimplementation on top of therollingbranchtest_input_aligner.cppcoverageDetails
This PR targets
ros2/message_filters:rollingfromUniflexAI/message_filters:rolling.The Python implementation follows the C++ InputAligner semantics referenced from the upstream
sa-input_alignerwork.Test coverage
The Python test file now covers 10 InputAligner cases, including the original C++ parity coverage plus Python-specific regression coverage for recent fixes:
Validation
Validated on a Rolling environment with all 10 Python InputAligner tests passing.