feat: dedicated callback thread, on_configured hook, send_config()#29
Merged
Conversation
All user-visible callbacks (on_connected, on_disconnected, on_configured, on_*_changed) now run on a per-ServiceInterface daemon thread (xbot-cb-*) instead of spawning a new thread per event. This means: - Callbacks are serialised and arrive in order - RPC calls (call_*) are safe inside any callback without deadlock New API: - on_configured: fires after connect + initial config transaction sent - send_config(): explicitly push register values when already connected - RegisterProxy no longer auto-sends on assignment while connected lcd_hello.py rewritten as a button-driven counter using on_configured and on_gpio_event_changed callbacks. gpio.json updated to two inputs. Add _join_callbacks() test helper; update tests that assert on async callback delivery to drain the queue before asserting.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR centralizes ServiceInterface callback execution on a background queue, stops auto-sending configuration when registers are written while connected (introducing send_config()), adds an on_configured lifecycle, refactors lifecycle/data dispatch to use the worker, updates the LCD example to event-driven handlers, and adjusts tests to await queued callbacks. ChangesEvent-Driven Callback System & Register Behavior
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xbot_service_interface_py/xbot_service_interface/interface.py (1)
492-541:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly fire
on_configuredafter a valid connected config push.After Line 557,
_active_schemaand_ioare still live, sosend_config()can re-enter this path and callsend_transaction()even whileconnectedisFalse. Lines 533-541 also enqueueon_configuredwhen required registers were skipped or a register failed to serialize, which breaks the new “service is fully usable” contract.🛠️ Suggested guard
def _on_config_request(self) -> None: schema = self._active_schema - if schema is None: - log.warning(f"Service {self._service_id} requested config but schema unavailable") + if schema is None or not self._connected or self._io is None: + log.warning( + f"Service {self._service_id} requested config while not fully connected") return @@ - chunks = [] - missing_required = [] + chunks = [] + missing_required = [] + serialization_failed = False @@ try: raw = pack_value(reg['type_str'], value, schema.enums_dict) chunks.append((reg['id'], raw)) except Exception as e: log.error(f"Cannot serialize register {reg['name']!r}: {e}") + serialization_failed = True @@ - if chunks and self._io is not None: + if chunks: self._io.send_transaction(self._service_id, chunks, is_config=True) log.info( f"Sent configuration for service {self._service_id} " f"({len(chunks)} registers)") @@ - configured_cbs = list(self._configured_callbacks) - if configured_cbs: + config_ready = not missing_required and not serialization_failed + configured_cbs = list(self._configured_callbacks) if config_ready else [] + if configured_cbs: def _fire(): for cb in configured_cbs: try: cb()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xbot_service_interface_py/xbot_service_interface/interface.py` around lines 492 - 541, _on_config_request builds chunks and currently enqueues on_configured callbacks even when config wasn't actually sent (missing_required, serialization errors, or disconnected), so change it to only fire configured callbacks after a successful send_transaction: track serialization failures (e.g. a boolean like serialize_failed) and only call self._io.send_transaction when self._io is not None and self.connected is True and chunks is non-empty and missing_required is empty and serialize_failed is False; then only after send_transaction completes (and self.connected still True) dispatch configured_cbs from _configured_callbacks. Update references in this method (_on_config_request, self._io.send_transaction, self.connected, self._active_schema, configured_cbs) accordingly to ensure callbacks are not enqueued on failure or when disconnected.
🧹 Nitpick comments (3)
xbot_service_interface_py/tests/test_interface.py (2)
459-470: ⚡ Quick winAvoid set-based ID checks here; it can hide duplicate register chunks.
Using a
setloses multiplicity, so duplicate sends could still pass. Assert chunk count plus exact IDs to lock in single-send-per-register behavior.Proposed assertion tightening
si.send_config() si._io.send_transaction.assert_called_once() _, chunks, *_ = si._io.send_transaction.call_args[0] - ids = {c[0] for c in chunks} - assert ids == {0, 1} # both Prefix (id=0) and EchoCount (id=1) sent + ids = [c[0] for c in chunks] + assert len(chunks) == 2 + assert sorted(ids) == [0, 1] # both Prefix (id=0) and EchoCount (id=1), once each🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xbot_service_interface_py/tests/test_interface.py` around lines 459 - 470, The test test_send_config_sends_all_registers currently converts chunk IDs to a set which hides duplicates; update the assertions after calling si.send_config() to ensure no duplicate register chunks are sent by checking the total number of chunks and the exact list/multiplicity of IDs from si._io.send_transaction call (inspect the chunks variable returned from the call_args for send_transaction) instead of using a set — e.g., assert len(chunks) equals expected number and that the sequence or sorted list of IDs equals [0,1] (or the expected repeated pattern) so each register ID appears exactly once.
250-257: ⚡ Quick winAssert the
on_configuredordering contract, not only invocation.This test currently proves the callback fires, but not that it fires after config is sent. That ordering is a core behavior for this PR.
Proposed test hardening
def test_on_configured_fires_on_config_request(self): si = make_si(connected=True) - cb = MagicMock() + events = [] + si._io.send_transaction.side_effect = lambda *args, **kwargs: events.append('send') + cb = MagicMock(side_effect=lambda: events.append('configured')) si.on_configured(cb) si._on_config_request() si._join_callbacks() cb.assert_called_once() + assert events == ['send', 'configured']🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xbot_service_interface_py/tests/test_interface.py` around lines 250 - 257, The test must assert ordering (callback runs after config is sent): register the callback via si.on_configured but make the callback a MagicMock with a side_effect that asserts the config was already sent (e.g., check a sentinel on the session such as si._config_sent or that si.send_config/send_config_message was called) so when si._on_config_request() and si._join_callbacks() run the side_effect verifies config-sent state before allowing the mock to complete; use the existing symbols si.on_configured, si._on_config_request, si._join_callbacks and the cb MagicMock with a side_effect to enforce the "after" ordering rather than just cb.assert_called_once().xbot_service_interface_py/xbot_service_interface/manager.py (1)
5-7: 💤 Low valueConsider clarifying thread startup timing.
The phrase "All four start when XbotServiceIo.start() is called" followed by "(xbot-cb-* starts at ServiceInterface construction time)" could confuse readers since one thread starts earlier than the others.
📝 Suggested clarification
-Four daemon threads are created per XbotServiceIo + ServiceInterface pair. -All four start when XbotServiceIo.start() is called (xbot-cb-* starts at -ServiceInterface construction time). +Four daemon threads are created per XbotServiceIo + ServiceInterface pair: +three start when XbotServiceIo.start() is called, and xbot-cb-* starts at +ServiceInterface construction time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xbot_service_interface_py/xbot_service_interface/manager.py` around lines 5 - 7, Update the misleading comment about thread startup timing to explicitly state which threads start when; change the phrasing around XbotServiceIo.start() and ServiceInterface construction so it reads something like: three daemon threads are created per XbotServiceIo + ServiceInterface pair and are started when XbotServiceIo.start() is called, while the xbot-cb-* callback thread is created and started during ServiceInterface construction; reference XbotServiceIo.start() and the ServiceInterface constructor (and the xbot-cb-* thread name) in the comment to make the distinction unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xbot_service_interface_py/examples/lcd_hello.py`:
- Around line 102-103: The post-clear delay after sending the 0x01 clear-display
command is too short (time.sleep(0.0005)) and can cause CGRAM writes to race;
increase this delay to at least 0.002 seconds (2 ms) or define a CLEAR_DELAY
constant and use time.sleep(CLEAR_DELAY) immediately after the self._cmd(0x01)
call (where the initialization/send sequence is performed) so the display has
time to complete the clear before subsequent CGRAM writes.
In `@xbot_service_interface_py/xbot_service_interface/interface.py`:
- Around line 209-241: The callback worker never gets a shutdown signal or join,
so _cb_thread, _cb_queue and the ServiceInterface instance leak; add an explicit
shutdown/close method (e.g., close() or shutdown()) that puts the None sentinel
into _cb_queue and joins _cb_thread, and ensure any public cleanup paths (or
__enter__/__exit__ or context manager support) call it; update
_dispatch/_join_callbacks usage if necessary to guard against dispatch after
shutdown and ensure _cb_worker still treats None as the termination signal and
returns so the thread can be joined.
---
Outside diff comments:
In `@xbot_service_interface_py/xbot_service_interface/interface.py`:
- Around line 492-541: _on_config_request builds chunks and currently enqueues
on_configured callbacks even when config wasn't actually sent (missing_required,
serialization errors, or disconnected), so change it to only fire configured
callbacks after a successful send_transaction: track serialization failures
(e.g. a boolean like serialize_failed) and only call self._io.send_transaction
when self._io is not None and self.connected is True and chunks is non-empty and
missing_required is empty and serialize_failed is False; then only after
send_transaction completes (and self.connected still True) dispatch
configured_cbs from _configured_callbacks. Update references in this method
(_on_config_request, self._io.send_transaction, self.connected,
self._active_schema, configured_cbs) accordingly to ensure callbacks are not
enqueued on failure or when disconnected.
---
Nitpick comments:
In `@xbot_service_interface_py/tests/test_interface.py`:
- Around line 459-470: The test test_send_config_sends_all_registers currently
converts chunk IDs to a set which hides duplicates; update the assertions after
calling si.send_config() to ensure no duplicate register chunks are sent by
checking the total number of chunks and the exact list/multiplicity of IDs from
si._io.send_transaction call (inspect the chunks variable returned from the
call_args for send_transaction) instead of using a set — e.g., assert
len(chunks) equals expected number and that the sequence or sorted list of IDs
equals [0,1] (or the expected repeated pattern) so each register ID appears
exactly once.
- Around line 250-257: The test must assert ordering (callback runs after config
is sent): register the callback via si.on_configured but make the callback a
MagicMock with a side_effect that asserts the config was already sent (e.g.,
check a sentinel on the session such as si._config_sent or that
si.send_config/send_config_message was called) so when si._on_config_request()
and si._join_callbacks() run the side_effect verifies config-sent state before
allowing the mock to complete; use the existing symbols si.on_configured,
si._on_config_request, si._join_callbacks and the cb MagicMock with a
side_effect to enforce the "after" ordering rather than just
cb.assert_called_once().
In `@xbot_service_interface_py/xbot_service_interface/manager.py`:
- Around line 5-7: Update the misleading comment about thread startup timing to
explicitly state which threads start when; change the phrasing around
XbotServiceIo.start() and ServiceInterface construction so it reads something
like: three daemon threads are created per XbotServiceIo + ServiceInterface pair
and are started when XbotServiceIo.start() is called, while the xbot-cb-*
callback thread is created and started during ServiceInterface construction;
reference XbotServiceIo.start() and the ServiceInterface constructor (and the
xbot-cb-* thread name) in the comment to make the distinction unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42480717-0d44-4abf-9c8a-dfb43e688be7
📒 Files selected for processing (5)
xbot_service_interface_py/examples/gpio.jsonxbot_service_interface_py/examples/lcd_hello.pyxbot_service_interface_py/tests/test_interface.pyxbot_service_interface_py/xbot_service_interface/interface.pyxbot_service_interface_py/xbot_service_interface/manager.py
- Add close() to stop the callback thread cleanly; guard _dispatch against calls after shutdown - Gate on_configured callbacks on successful config send: suppress when serialization errors occur or io is unavailable; fire when no registers need sending (treated as success) - Increase HD44780 clear-display post-command delay 0.5 ms → 2 ms to satisfy the ≥1.52 ms datasheet requirement before CGRAM writes - Replace set-based chunk ID assertion with len + sorted list to catch duplicate register IDs - Add test_on_configured_fires_after_config_sent: asserts send_transaction completes before the configured callback runs - Add test_on_configured_suppressed_on_serialize_error - Fix manager.py thread startup comment to unambiguously distinguish XbotServiceIo.start() threads from the ServiceInterface constructor thread
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
on_connected,on_disconnected,on_configured,on_*_changed) now run on a single per-ServiceInterfacedaemon thread (xbot-cb-<id>) instead of spawning a new thread per event — callbacks are serialised and arrive in ordercall_*) are safe inside any callback without deadlocking the IO recv threadon_configuredhook fires after connect + initial config transaction is sent — the service is fully usable at that pointsend_config()method to explicitly push register values while already connectedRegisterProxyno longer auto-sends on assignment while connected (was sending incomplete config)manager.pygains a threading model reference doclcd_hello.pyrewritten as a button-driven counter usingon_configuredandon_gpio_event_changed;gpio.jsonupdated to two input GPIOs.Test plan
pytest tests/)on_configuredfires after service connect and config sendon_configuredcomplete without deadlocksend_config()sends all registers in one transactionSummary by CodeRabbit
New Features
Changes
Tests
Documentation