feat(someip): add server/provider side (offer services, answer RPC, publish events) - #913
feat(someip): add server/provider side (offer services, answer RPC, publish events)#913kirkbrauer wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe SOME/IP driver now supports provider/server mode. It can offer services, return configured RPC responses, publish events, set fields, list offers, manage server state, and use provider configuration and tests. ChangesSOME/IP provider support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SomeIpDriverClient
participant SomeIp
participant OsipServer
Client->>SomeIpDriverClient: configure provider behavior
SomeIpDriverClient->>SomeIp: call server API
SomeIp->>OsipServer: start, offer service, or register handler
OsipServer-->>SomeIp: deliver RPC request or subscriber event
SomeIp-->>Client: return response or publish event
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@vtz — as the original author of this SOME/IP driver, your review would be much appreciated. This adds the server/provider side (offer services via SD, answer RPC with canned responses, publish events) on top of your client-side implementation, wrapping the |
…ublish events) The SOME/IP driver was client-only. opensomeip already ships a full server-side API (SomeIpServer / SdServer / RpcServer / EventPublisher), so expose it through the driver so a Jumpstarter exporter can act as a simulated ECU that a device-under-test's SOME/IP client talks to. New exported verbs (driver + client): - start_server / stop_server: server lifecycle (lazily started on first offer) - offer_service / stop_offer_service / list_offered_services: SD offering - set_method_response / clear_method_response: canned RPC responses. RPC handlers run in the exporter process and cannot call back to the Jumpstarter client per request, so responses are configured declaratively per (service_id, method_id); this maps cleanly onto getter-style methods. - register_event / publish_event / set_field: event group notifications and cached field events. Adds a SomeIpOfferedService model, README "Server / Provider" usage section, a provider exporter example, and 13 server-side unit tests (83 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
756f728 to
f07ee6b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py (1)
359-369: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
try/except ValueErrorover the enum internal_value2member_map_.
ReturnCode._value2member_map_is a CPython enum implementation detail. Constructing the enum in atry/exceptis the documented idiom and is robust to future enum internals.♻️ Optional refactor
- payload, return_code = self._method_responses.get(key, (b"", int(ReturnCode.E_OK))) - rc = ReturnCode(return_code) if return_code in ReturnCode._value2member_map_ else ReturnCode.E_NOT_OK + payload, return_code = self._method_responses.get(key, (b"", int(ReturnCode.E_OK))) + try: + rc = ReturnCode(return_code) + except ValueError: + rc = ReturnCode.E_NOT_OK🤖 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 `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py` around lines 359 - 369, Update the ReturnCode conversion in handler to construct ReturnCode(return_code) inside a try block and catch ValueError to fall back to ReturnCode.E_NOT_OK. Remove the direct use of ReturnCode._value2member_map_, while preserving the existing response/error message type behavior.
🤖 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.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py`:
- Around line 359-369: Update the ReturnCode conversion in handler to construct
ReturnCode(return_code) inside a try block and catch ValueError to fall back to
ReturnCode.E_NOT_OK. Remove the direct use of ReturnCode._value2member_map_,
while preserving the existing response/error message type behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d0c1305-d16a-4d5a-83aa-e3bbee1e30f0
📒 Files selected for processing (6)
python/packages/jumpstarter-driver-someip/README.mdpython/packages/jumpstarter-driver-someip/examples/exporter.yamlpython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/client.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/common.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py
|
I will remove my review to let @vtz come and review. |
vtz
left a comment
vtz
left a comment
There was a problem hiding this comment.
Thanks for this. Nice addition. Exposing the provider side so an exporter can act as a simulated ECU fits Open SOME/IP and opensomeip well, and one driver ≈ one ECU is the right Jumpstarter shape.
Inline comments cover the small cleanup/test nits. Separately, worth noting for later (not blockers): Fire&Forget and the full Field getter/setter/notifier model from the spec aren’t in scope here (canned RPC + set_field/events cover the usual simulator path).
Great work!
|
|
||
| @export | ||
| @validate_call(validate_return=True) | ||
| def publish_event(self, service_id: int, event_id: int, payload: SomeIpPayload) -> None: |
There was a problem hiding this comment.
service_id is accepted here but never forwarded (opensomeip only takes event_id). Please document that it’s unused for API symmetry, or remove it so callers aren’t misled.
There was a problem hiding this comment.
@kirkbrauer have you worked on this or do you want to keep it as is?
- Revert formatting-only changes to existing client methods - Reject reserved instance IDs (0x0000, 0xFFFF) in offer_service - Clear canned method responses in stop_server so stale payloads do not survive a server restart - Document that service_id is unused (API symmetry) in publish_event and set_field on both driver and client - Add stateful loopback server tests (StatefulOsipServer + LoopbackOsipClient) covering offer + discovery + canned RPC responses, event publishing, and state clearing across stop_server Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Restore main's original formatting in driver.py and driver_test.py (hex literal casing, line wrapping) so the diff only contains the server/provider feature changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py (1)
1233-1239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert
message_typein the error test.The driver returns
MessageType.ERRORwhen the return code is notE_OK(driver.py Lines 365-375). The test assertsreturn_codeonly. Add themessage_typeassertion to cover the error branch fully.♻️ Proposed test tightening
resp = loopback_client.rpc_call(0x1801, 0x0006, b"") assert resp.return_code == 0x01 + assert resp.message_type == int(MessageType.ERROR)🤖 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 `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py` around lines 1233 - 1239, Update test_loopback_method_error_return_code to also assert that resp.message_type is MessageType.ERROR when the non-success return_code is returned, while preserving the existing return_code assertion.python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py (1)
426-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
stop()drop handler and event registrations.
stop()clears_offeredonly. It keepshandlers,registered_events, andfields. A real server restart drops all registrations. The loopback tests patchOsipServerwith a fixedreturn_value, so the same instance is reused afterstop_server(). Stale handlers therefore survive a restart that would clear them in production.This weakens
test_loopback_stop_server_clears_canned_responses: the post-restartrpc_callsucceeds only because the stale handler remains registered. A driver defect that skips method re-registration after a restart would not be detected.♻️ Proposed fix to align the double with real restart semantics
def stop(self): self._started = False self._offered.clear() + self.handlers.clear() + self.registered_events.clear() + self.fields.clear()🤖 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 `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py` around lines 426 - 428, Update the loopback server double’s stop() method to clear handlers, registered_events, and fields in addition to _offered, matching production restart semantics. Ensure all registration state is removed before the same OsipServer instance can be reused after stop_server().
🤖 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
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py`:
- Around line 485-490: Update the OsipMessage construction in the loopback
request helper to set explicit SOME/IP response headers: provide an
interface_version and message_type matching the values expected by
SomeIp._make_method_handler. Preserve the existing message_id, request_id, and
payload behavior.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py`:
- Around line 426-428: Update the loopback server double’s stop() method to
clear handlers, registered_events, and fields in addition to _offered, matching
production restart semantics. Ensure all registration state is removed before
the same OsipServer instance can be reused after stop_server().
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py`:
- Around line 1233-1239: Update test_loopback_method_error_return_code to also
assert that resp.message_type is MessageType.ERROR when the non-success
return_code is returned, while preserving the existing return_code assertion.
🪄 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: a8fdedc9-d154-424e-8422-0b3a0073b41f
📒 Files selected for processing (4)
python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/client.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py
|
sent to merge queue again, the python failure was unrelated, related to renode. |
|
@mangelajo Humm, looks like it failed again, I'll wait for @vtz 's review first and then we can retry the merge queue :) |
vtz
left a comment
vtz
left a comment
There was a problem hiding this comment.
I'm approving. Consider a follow up ticket for Fire&Forget, full Field getter/setter/notifier.
|
|
||
| @export | ||
| @validate_call(validate_return=True) | ||
| def publish_event(self, service_id: int, event_id: int, payload: SomeIpPayload) -> None: |
There was a problem hiding this comment.
@kirkbrauer have you worked on this or do you want to keep it as is?
aef4de1 to
6ee3ccd
Compare
3 participants
The SOME/IP driver is currently client-only.
opensomeipalready ships a full server-side API (SomeIpServer/SdServer/RpcServer/EventPublisher), so this exposes it through the driver so a Jumpstarter exporter can act as a simulated ECU that a device-under-test's SOME/IP client talks to.New exported verbs (driver + client)
start_server/stop_server— server lifecycle (lazily started on first offer)offer_service/stop_offer_service/list_offered_services— SD offeringset_method_response/clear_method_response— canned RPC responsesregister_event/publish_event/set_field— event-group notifications and cached field eventsDesign note
RPC handlers run inside the exporter process (opensomeip receive thread) and cannot call back to the Jumpstarter client per request, so responses are configured declaratively per
(service_id, method_id)rather than via a per-request callback. This maps naturally onto getter-style SOME/IP methods, updating the response changes what a client reads without re-registration.Also included
SomeIpOfferedServicemodel for server-side introspectionopensomeip.SomeIpServer); full package suite: 83 passed, 2 skippedThe client side is unchanged; all additions are additive.