feat: add SOME/IP driver wrapping opensomeip Python binding#391
feat: add SOME/IP driver wrapping opensomeip Python binding#391vtz wants to merge 7 commits intojumpstarter-dev:mainfrom
Conversation
Add jumpstarter-driver-someip package providing remote access to SOME/IP protocol operations (RPC, service discovery, raw messaging, events) via the opensomeip Python binding (https://pypi.org/project/opensomeip/). Follows the same driver pattern as DoIP, UDS, and XCP drivers with pydantic dataclass driver, @export/@validate_call methods, DriverClient subclass, and comprehensive tests (16 passing, 2 integration skipped). Made-with: Cursor
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new jumpstarter-driver-someip package: Pydantic SOME/IP data models, a SomeIp driver wrapping opensomeip, a typed DriverClient façade, test fixtures and suites (unit + optional integration), docs and example manifest, plus packaging and entry-point metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant DriverClient as SomeIpDriverClient
participant Driver as SomeIp
participant Osip as OsipClient
participant Remote as RemoteService
Client->>DriverClient: rpc_call(service_id, method_id, payload: bytes)
DriverClient->>Driver: rpc_call(..., payload_hex, timeout)
Driver->>Osip: call(MessageId, payload_bytes, timeout)
Osip->>Remote: send RPC request
Remote-->>Osip: RPC response bytes
Osip-->>Driver: Message object
Driver-->>DriverClient: SomeIpMessageResponse (payload hex)
DriverClient-->>Client: SomeIpMessageResponse
sequenceDiagram
participant Client as Client
participant DriverClient as SomeIpDriverClient
participant Driver as SomeIp
participant Osip as OsipClient
participant SD as SomeIP_SD
Client->>DriverClient: find_service(service_id, instance_id)
DriverClient->>Driver: find_service(...)
Driver->>Osip: start discovery (callback)
Osip->>SD: SD request (network)
SD-->>Osip: discovery replies (callbacks)
Osip-->>Driver: discovery callbacks with entries
Driver-->>DriverClient: list[SomeIpServiceEntry]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py (2)
88-97: Test doesn't verifyservice_idis used.The test calls
client.subscribe_eventgroup(0x1234, 1)but the assertion only checksmock_client.subscribe_events.assert_called_once_with(1). Theservice_idparameter (0x1234) isn't verified in the mock call. Ifservice_idis expected to be passed to the underlying library, consider adding an assertion for it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py` around lines 88 - 97, The test test_someip_subscribe_eventgroup currently only asserts mock_client.subscribe_events was called with (1) but doesn't verify the service_id; update the assertion to include the service_id passed through by SomeIp.subscribe_eventgroup (e.g., assert mock_client.subscribe_events.assert_called_once_with(0x1234, 1) or inspect mock_client.subscribe_events.call_args) so the test confirms SomeIp (constructor SomeIp(...) and method subscribe_eventgroup) forwards the service_id to OsipClient.subscribe_events.
26-44: Mock setup accesses internal implementation details.The mock setup directly accesses
mock.transport.receiver._sync_queueandmock.event_subscriber.notifications.return_value._sync_queue, which are internal implementation details ofopensomeip. This tight coupling could cause test breakage if the library's internals change.Consider documenting this dependency or abstracting it if feasible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py` around lines 26 - 44, The test helper _make_mock_osip_client currently reaches into opensomeip internals by setting mock.transport.receiver._sync_queue and mock.event_subscriber.notifications.return_value._sync_queue; change the mock to use public-facing behavior instead (e.g., set the transport receiver's public receive/next_message method return value or configure event_subscriber.notifications to return a mock receiver whose public read/next/receive method yields mock_response) or wrap this internal wiring behind a clear helper with a comment documenting the dependency; update references inside _make_mock_osip_client so it configures the receiver via public methods/properties (not _sync_queue) and/or add a comment explaining the internal assumption if abstraction isn't feasible.python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py (1)
36-48: Verify the struct format string spacing.The format string
"!HHIHH BBBB"contains a space betweenHHIHHandBBBB. While Python'sstructmodule ignores whitespace in format strings, this is unusual and could be confusing. Consider removing the space for clarity:"!HHIHHBBBB".Suggested fix
header = struct.pack( - "!HHIHH BBBB", + "!HHIHHBBBB", service_id, method_id,And similarly on line 65:
service_id, method_id, length, client_id, session_id, proto_ver, iface_ver, msg_type, ret_code = struct.unpack( - "!HHIHH BBBB", header + "!HHIHHBBBB", header )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py` around lines 36 - 48, The struct format string used in struct.pack in conftest.py currently includes an unexpected space ("!HHIHH BBBB"); update the format to remove the space (use "!HHIHHBBBB") in the header construction inside the function where header = struct.pack(...) is called, and do the same for the similar struct.pack call referenced around line 65 so both format strings have no embedded whitespace for clarity.python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/common.py (1)
21-33: Consider adding hex validation topayloadfield.
SomeIpPayloadvalidates thatdatais hex-encoded, butSomeIpMessageResponse.payload(andSomeIpEventNotification.payload) are plainstrwithout validation. If these can contain arbitrary data from the wire, this is fine. However, if they're expected to be hex-encoded as the comment suggests, consider reusing the validator or inheriting fromSomeIpPayload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/common.py` around lines 21 - 33, SomeIpMessageResponse and SomeIpEventNotification declare payload as plain str but the comment says it's hex-encoded; update these models to validate/reuse the existing SomeIpPayload validator by either (A) changing the field type from str to SomeIpPayload (so payload uses SomeIpPayload.data and its hex validation) or (B) add a pydantic validator on SomeIpMessageResponse.payload and SomeIpEventNotification.payload that reuses the hex-check logic from SomeIpPayload (e.g., call the same validation helper or import the validator function) so incoming payload values are guaranteed to be hex-encoded; refer to the class names SomeIpMessageResponse, SomeIpEventNotification, and SomeIpPayload to locate where to change the field type or add the validator.python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py (3)
137-164:time.sleep(timeout)always waits the full duration.Line 163 blocks for the entire timeout regardless of whether services are found. For service discovery this may be intentional (to gather all responses), but if early return is desirable when at least one service is found, consider using an event or condition variable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py` around lines 137 - 164, The find_service implementation blocks with time.sleep(timeout) which always waits the full duration; update find_service to use a threading.Event or Condition instead so on_found (the callback) can signal discovery and unblock early when appropriate: create an event before calling self._osip_client.find, have on_found append to found and call event.set() (or notify a condition) and then replace time.sleep(timeout) with event.wait(timeout) (or condition.wait_for) to return as soon as at least one service is found or timeout elapses; ensure cleanup and thread-safety around the found list and callback registration in find_service and on_found.
62-76: Invalidtransport_modevalues silently default to UDP.Line 66 only checks for
"TCP"explicitly, meaning any invalid value (e.g.,"FOO","tcpp") silently becomes UDP without warning. Consider validating the value explicitly.♻️ Proposed fix to validate transport_mode
def __post_init__(self): if hasattr(super(), "__post_init__"): super().__post_init__() - mode = TransportMode.TCP if self.transport_mode.upper() == "TCP" else TransportMode.UDP + transport_upper = self.transport_mode.upper() + if transport_upper == "TCP": + mode = TransportMode.TCP + elif transport_upper == "UDP": + mode = TransportMode.UDP + else: + raise ValueError(f"Invalid transport_mode: {self.transport_mode!r}. Must be 'TCP' or 'UDP'.") config = ClientConfig(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py` around lines 62 - 76, In __post_init__, do not silently treat unknown self.transport_mode values as UDP; instead validate self.transport_mode (case-insensitive) against the allowed values "TCP" and "UDP", set mode to TransportMode.TCP or TransportMode.UDP only for those valid values, and raise a clear ValueError (or log and raise) including the invalid value when it’s neither "TCP" nor "UDP"; update the logic around TransportMode, mode, and ClientConfig initialization in the same method to use the validated mode.
122-133: Accessing private_sync_queueattribute is fragile.Line 130 accesses
receiver._sync_queue, which is an internal implementation detail of opensomeip. This could break if the library's internals change. Consider checking if opensomeip provides a public API for synchronous message receiving.opensomeip python receive message API🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py` around lines 122 - 133, The method receive_message currently accesses the private attribute receiver._sync_queue (fragile) — update receive_message to prefer a public opensomeip API for synchronous receive (e.g., a documented receiver.receive or receiver.get_message method) if available, and only fall back to accessing _sync_queue via safe existence checks (use getattr(receiver, "receive", None) or getattr(receiver, "_sync_queue", None)) while raising a clear error if no public API or queue is present; reference the receive_message method, the receiver object, the private _sync_queue attribute, and the _message_to_response conversion so you ensure messages are obtained via the public call when possible and converted with _message_to_response otherwise, with an explicit, helpful exception if neither option exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py`:
- Around line 203-208: The reconnect method can leave the client stopped if
self._osip_client.stop() raises; update reconnect (function reconnect) to catch
exceptions from self._osip_client.stop(), log or record the error similarly to
the close() implementation, and then always attempt self._osip_client.start();
if start() then fails, propagate or handle that error per close()’s pattern
(cleanup/raise) so the driver does not remain in an inconsistent stopped state
after a failed reconnect.
- Around line 166-176: The methods subscribe_eventgroup and
unsubscribe_eventgroup currently declare a service_id parameter that is never
used and not passed to
_osip_client.subscribe_events/_osip_client.unsubscribe_events; remove the unused
service_id parameter from both method signatures, update their
docstrings/signature annotations to only accept eventgroup_id: int -> None, and
adjust any decorator usage (export/validate_call) or type hints if they
reference the removed parameter; ensure references to service_id are removed
inside the methods and update any callers/tests to stop passing service_id so
the API matches the opensomeip calls.
In `@python/packages/jumpstarter-driver-someip/README.md`:
- Around line 91-94: The documentation is inconsistent: the API table lists
find_service(service_id, instance_id, timeout) but the example calls
find_service(0x1234, timeout=3.0); update the README to make the signature
consistent with the implementation by either (A) documenting that instance_id is
optional (e.g., instance_id=None) and leaving the example as-is, or (B) changing
the example to pass an explicit instance_id (e.g., find_service(0x1234, 0x0001,
timeout=3.0)); ensure the function name find_service and its parameters in the
API table and the example match exactly.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/common.py`:
- Around line 21-33: SomeIpMessageResponse and SomeIpEventNotification declare
payload as plain str but the comment says it's hex-encoded; update these models
to validate/reuse the existing SomeIpPayload validator by either (A) changing
the field type from str to SomeIpPayload (so payload uses SomeIpPayload.data and
its hex validation) or (B) add a pydantic validator on
SomeIpMessageResponse.payload and SomeIpEventNotification.payload that reuses
the hex-check logic from SomeIpPayload (e.g., call the same validation helper or
import the validator function) so incoming payload values are guaranteed to be
hex-encoded; refer to the class names SomeIpMessageResponse,
SomeIpEventNotification, and SomeIpPayload to locate where to change the field
type or add the validator.
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py`:
- Around line 36-48: The struct format string used in struct.pack in conftest.py
currently includes an unexpected space ("!HHIHH BBBB"); update the format to
remove the space (use "!HHIHHBBBB") in the header construction inside the
function where header = struct.pack(...) is called, and do the same for the
similar struct.pack call referenced around line 65 so both format strings have
no embedded whitespace for clarity.
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py`:
- Around line 88-97: The test test_someip_subscribe_eventgroup currently only
asserts mock_client.subscribe_events was called with (1) but doesn't verify the
service_id; update the assertion to include the service_id passed through by
SomeIp.subscribe_eventgroup (e.g., assert
mock_client.subscribe_events.assert_called_once_with(0x1234, 1) or inspect
mock_client.subscribe_events.call_args) so the test confirms SomeIp (constructor
SomeIp(...) and method subscribe_eventgroup) forwards the service_id to
OsipClient.subscribe_events.
- Around line 26-44: The test helper _make_mock_osip_client currently reaches
into opensomeip internals by setting mock.transport.receiver._sync_queue and
mock.event_subscriber.notifications.return_value._sync_queue; change the mock to
use public-facing behavior instead (e.g., set the transport receiver's public
receive/next_message method return value or configure
event_subscriber.notifications to return a mock receiver whose public
read/next/receive method yields mock_response) or wrap this internal wiring
behind a clear helper with a comment documenting the dependency; update
references inside _make_mock_osip_client so it configures the receiver via
public methods/properties (not _sync_queue) and/or add a comment explaining the
internal assumption if abstraction isn't feasible.
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py`:
- Around line 137-164: The find_service implementation blocks with
time.sleep(timeout) which always waits the full duration; update find_service to
use a threading.Event or Condition instead so on_found (the callback) can signal
discovery and unblock early when appropriate: create an event before calling
self._osip_client.find, have on_found append to found and call event.set() (or
notify a condition) and then replace time.sleep(timeout) with
event.wait(timeout) (or condition.wait_for) to return as soon as at least one
service is found or timeout elapses; ensure cleanup and thread-safety around the
found list and callback registration in find_service and on_found.
- Around line 62-76: In __post_init__, do not silently treat unknown
self.transport_mode values as UDP; instead validate self.transport_mode
(case-insensitive) against the allowed values "TCP" and "UDP", set mode to
TransportMode.TCP or TransportMode.UDP only for those valid values, and raise a
clear ValueError (or log and raise) including the invalid value when it’s
neither "TCP" nor "UDP"; update the logic around TransportMode, mode, and
ClientConfig initialization in the same method to use the validated mode.
- Around line 122-133: The method receive_message currently accesses the private
attribute receiver._sync_queue (fragile) — update receive_message to prefer a
public opensomeip API for synchronous receive (e.g., a documented
receiver.receive or receiver.get_message method) if available, and only fall
back to accessing _sync_queue via safe existence checks (use getattr(receiver,
"receive", None) or getattr(receiver, "_sync_queue", None)) while raising a
clear error if no public API or queue is present; reference the receive_message
method, the receiver object, the private _sync_queue attribute, and the
_message_to_response conversion so you ensure messages are obtained via the
public call when possible and converted with _message_to_response otherwise,
with an explicit, helpful exception if neither option exists.
🪄 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
Run ID: 09554097-1816-45a1-9399-849699ea85a2
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
python/docs/source/reference/package-apis/drivers/someip.mdpython/packages/jumpstarter-driver-someip/.gitignorepython/packages/jumpstarter-driver-someip/README.mdpython/packages/jumpstarter-driver-someip/examples/exporter.yamlpython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/__init__.pypython/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/conftest.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.pypython/packages/jumpstarter-driver-someip/pyproject.toml
python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py
Show resolved
Hide resolved
python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py
Show resolved
Hide resolved
- Validate transport_mode explicitly, reject invalid values with ValueError - Remove unused service_id parameter from subscribe/unsubscribe_eventgroup - Wrap private _sync_queue access in _receive_from_queue helper with getattr guard - Use threading.Event in find_service for early return instead of time.sleep - Catch stop() exceptions in reconnect() to avoid inconsistent state - Add hex validation to payload fields in response/notification models - Remove whitespace from struct format strings in conftest.py - Document internal opensomeip dependency in test mock setup - Fix README API table vs example code inconsistency - Add test for reconnect surviving stop() failure Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py (2)
157-186: Consider race condition and multi-result handling in service discovery.The current implementation sets the event after the first callback, which causes
event.wait()to return early. Iffind()can discover multiple services, only the first one may be returned before the timeout expires.If returning early after the first discovery is intentional (common for "find any matching service" use cases), consider documenting this behavior. If all matches within the timeout should be returned, the event should not be set in the callback.
💡 Option A: Document early-return behavior
def find_service( self, service_id: int, instance_id: int = 0xFFFF, timeout: float = 5.0, ) -> list[SomeIpServiceEntry]: - """Find services via SOME/IP-SD.""" + """Find services via SOME/IP-SD. + + Returns immediately after the first service is discovered, or after + the timeout if no services are found. + """💡 Option B: Wait for full timeout to collect all results
- def on_found(svc: ServiceInstance) -> None: - found.append( - SomeIpServiceEntry( - service_id=svc.service_id, - instance_id=svc.instance_id, - major_version=svc.major_version, - minor_version=svc.minor_version, - ) - ) - event.set() + def on_found(svc: ServiceInstance) -> None: + found.append( + SomeIpServiceEntry( + service_id=svc.service_id, + instance_id=svc.instance_id, + major_version=svc.major_version, + minor_version=svc.minor_version, + ) + ) self._osip_client.find(service, callback=on_found) - event.wait(timeout=timeout) + import time + time.sleep(timeout) return found🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py` around lines 157 - 186, The find_service implementation currently sets event in the on_found callback which wakes event.wait() after the first discovery; decide whether to keep early-return or collect all matches: if you want all matches within the timeout, remove the event.set() call in on_found and rely on event.wait(timeout) to time out (or replace event with a simple time-based loop) so every callback appends to found without waking early; if early-return is intended, update the find_service docstring to explicitly state "returns the first discovered service immediately" to avoid ambiguity; relevant symbols: find_service, on_found, self._osip_client.find, event.
44-57: Pragmatic and necessary workaround with good safeguards in place.The code correctly documents reliance on opensomeip's internal
_sync_queueattribute (verified: opensomeip Python library lacks a public blocking receive API), and includes a defensive check that fails fast if the internal API changes. This approach is sound.Consider adding a TODO comment noting the reliance on the private API and suggesting removal once opensomeip provides a public blocking-with-timeout method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py` around lines 44 - 57, Add a TODO comment in the _receive_from_queue function noting that it relies on the private opensomeip MessageReceiver attribute _sync_queue and should be removed or replaced once opensomeip provides a public blocking-with-timeout receive API; keep the existing defensive check for getattr(receiver, "_sync_queue", None) and the TimeoutError behavior, but add a concise TODO/TFIXME above the sync_queue retrieval referencing _sync_queue and the opensomeip library.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py`:
- Around line 157-186: The find_service implementation currently sets event in
the on_found callback which wakes event.wait() after the first discovery; decide
whether to keep early-return or collect all matches: if you want all matches
within the timeout, remove the event.set() call in on_found and rely on
event.wait(timeout) to time out (or replace event with a simple time-based loop)
so every callback appends to found without waking early; if early-return is
intended, update the find_service docstring to explicitly state "returns the
first discovered service immediately" to avoid ambiguity; relevant symbols:
find_service, on_found, self._osip_client.find, event.
- Around line 44-57: Add a TODO comment in the _receive_from_queue function
noting that it relies on the private opensomeip MessageReceiver attribute
_sync_queue and should be removed or replaced once opensomeip provides a public
blocking-with-timeout receive API; keep the existing defensive check for
getattr(receiver, "_sync_queue", None) and the TimeoutError behavior, but add a
concise TODO/TFIXME above the sync_queue retrieval referencing _sync_queue and
the opensomeip library.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb4abd54-3fb5-40d8-acef-7f5c010c8f21
📒 Files selected for processing (6)
python/packages/jumpstarter-driver-someip/README.mdpython/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/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 due to trivial changes (1)
- python/packages/jumpstarter-driver-someip/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py
- python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/common.py
- Add StatefulOsipClient in conftest.py: a drop-in replacement for opensomeip.SomeIpClient that enforces SOME/IP state rules (connection lifecycle, service registry, event subscriptions, message ordering) - Add 20 new stateful integration tests covering multi-step workflows: - RPC session: discover -> call -> verify -> disconnect - Messaging: send -> receive -> reconnect -> send again - Events: subscribe -> receive -> unsubscribe lifecycle - Service discovery: wildcard, specific instance, version, not-found - Connection management: close + reconnect, reconnect resets subs - Composite: discover -> RPC -> events -> cleanup - Add receive_event timeout error test (was missing vs receive_message) - Update README to match DoIP/XCP documentation quality with: - Multiple configuration examples (UDP + TCP) - API reference section with bullet-point descriptions - Separate example sections per feature area - Connection management example Made-with: Cursor
Unit tests: - test_someip_find_service: verifies find() call and callback wiring - test_someip_find_service_no_results: empty result when no match - test_someip_find_service_forwards_instance_id: verifies instance_id is passed through to the ServiceInstance Stateful tests: - test_stateful_find_different_services: independent results per service - test_stateful_find_service_dynamic_registration: services added at runtime appear in subsequent discoveries - test_stateful_find_service_dynamic_unregistration: removed services no longer appear - test_stateful_find_service_after_reconnect: registry persists - test_stateful_find_service_default_instance_wildcard: default 0xFFFF matches all instances - test_stateful_discover_then_rpc_to_each_instance: discover -> call Also adds register_service/unregister_service helpers to StatefulOsipClient for dynamic registry manipulation in tests. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py`:
- Around line 210-212: The test currently checks absolute call counts which can
be satisfied by prior setup; modify the assertions in the test around
client.reconnect() to record pre-call counts (e.g., prev_start =
mock_client.start.call_count and prev_stop = mock_client.stop.call_count), call
client.reconnect(), then assert that mock_client.stop.call_count >= prev_stop +
1 and mock_client.start.call_count >= prev_start + 1 to ensure reconnect()
actually invoked stop() and start() again; locate these changes in the test
where client.reconnect() is called and adjust the assertions accordingly.
🪄 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
Run ID: 7cda8b56-b357-4550-856c-a6d2aa556cf6
📒 Files selected for processing (3)
python/packages/jumpstarter-driver-someip/README.mdpython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-driver-someip/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py
| client.reconnect() | ||
| assert mock_client.stop.call_count >= 1 | ||
| assert mock_client.start.call_count >= 1 |
There was a problem hiding this comment.
Strengthen reconnect assertions to avoid false positives.
This currently passes even if reconnect() does not call start() again (because initial startup already increments start.call_count). Assert delta from pre-call counts instead.
Suggested test hardening
`@patch`("jumpstarter_driver_someip.driver.OsipClient")
def test_someip_reconnect(mock_osip_cls):
mock_client = _make_mock_osip_client()
mock_osip_cls.return_value = mock_client
driver = SomeIp(host="127.0.0.1", port=30490)
with serve(driver) as client:
+ stop_before = mock_client.stop.call_count
+ start_before = mock_client.start.call_count
client.reconnect()
- assert mock_client.stop.call_count >= 1
- assert mock_client.start.call_count >= 1
+ assert mock_client.stop.call_count == stop_before + 1
+ assert mock_client.start.call_count == start_before + 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py`
around lines 210 - 212, The test currently checks absolute call counts which can
be satisfied by prior setup; modify the assertions in the test around
client.reconnect() to record pre-call counts (e.g., prev_start =
mock_client.start.call_count and prev_stop = mock_client.stop.call_count), call
client.reconnect(), then assert that mock_client.stop.call_count >= prev_stop +
1 and mock_client.start.call_count >= prev_start + 1 to ensure reconnect()
actually invoked stop() and start() again; locate these changes in the test
where client.reconnect() is called and adjust the assertions accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py`:
- Around line 253-260: The fake discovery currently created by
_FakeServiceInstance does not enforce version constraints: update the fake
registry/find() implementation(s) to also compare major_version and
minor_version on _FakeServiceInstance when the lookup supplies version filters
(and treat unspecified version arguments as wildcards), so that callbacks only
fire for instances matching service_id, instance_id, and the requested
major/minor versions; apply the same change to the other find-like
implementations referenced around the other occurrences so all discovery-variant
tests respect versioned matching.
- Line 290: The fake record currently stores subscriptions in
self._subscribed_eventgroups: set[int] and never checks it when sending events;
change this to track subscriptions per service/instance (e.g.,
self._subscribed_eventgroups: dict[tuple[int,int], set[int]] keyed by
(service_id, instance_id)) and update the subscribe/unsubscribe handlers to
add/remove eventgroup IDs under the correct (service_id, instance_id) key; then,
in the event delivery paths referenced around 350-356 and 360-363 (the methods
that inject/deliver notifications), consult this map and only deliver an
injected event if its eventgroup ID is present for that service/instance. Ensure
subscribe/unsubscribe create empty sets as needed and clean up keys when empty.
🪄 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
Run ID: 210547f9-4069-4b90-a976-41e3ff38480b
📒 Files selected for processing (3)
python/packages/jumpstarter-driver-someip/README.mdpython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py
✅ Files skipped from review due to trivial changes (2)
- python/packages/jumpstarter-driver-someip/README.md
- python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py
| class _FakeServiceInstance: | ||
| """Mimics ``opensomeip.sd.ServiceInstance``.""" | ||
|
|
||
| def __init__(self, service_id: int, instance_id: int, major_version: int = 1, minor_version: int = 0): | ||
| self.service_id = service_id | ||
| self.instance_id = instance_id | ||
| self.major_version = major_version | ||
| self.minor_version = minor_version |
There was a problem hiding this comment.
Honor version constraints during fake service discovery.
find() only filters on service_id and instance_id, even though the registry carries major_version / minor_version. A lookup for a specific version will still callback every matching instance, so the discovery-variant tests can pass without the driver handling versioned matches correctly.
Also applies to: 284-288, 341-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py`
around lines 253 - 260, The fake discovery currently created by
_FakeServiceInstance does not enforce version constraints: update the fake
registry/find() implementation(s) to also compare major_version and
minor_version on _FakeServiceInstance when the lookup supplies version filters
(and treat unspecified version arguments as wildcards), so that callbacks only
fire for instances matching service_id, instance_id, and the requested
major/minor versions; apply the same change to the other find-like
implementations referenced around the other occurrences so all discovery-variant
tests respect versioned matching.
| _FakeServiceInstance(0x5678, 0x0001), | ||
| ] | ||
|
|
||
| self._subscribed_eventgroups: set[int] = set() |
There was a problem hiding this comment.
Make injected events respect active subscriptions.
The fake records subscribed eventgroups but never uses that state when delivering notifications. As written, a test can still receive an event before subscribe or after unsubscribe, and the bare set[int] cannot distinguish the same eventgroup ID on different services/instances.
Also applies to: 350-356, 360-363
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py`
at line 290, The fake record currently stores subscriptions in
self._subscribed_eventgroups: set[int] and never checks it when sending events;
change this to track subscriptions per service/instance (e.g.,
self._subscribed_eventgroups: dict[tuple[int,int], set[int]] keyed by
(service_id, instance_id)) and update the subscribe/unsubscribe handlers to
add/remove eventgroup IDs under the correct (service_id, instance_id) key; then,
in the event delivery paths referenced around 350-356 and 360-363 (the methods
that inject/deliver notifications), consult this map and only deliver an
injected event if its eventgroup ID is present for that service/instance. Ensure
subscribe/unsubscribe create empty sets as needed and clean up keys when empty.
Add SOME/IP entry to the Automotive Diagnostics Drivers section in drivers/index.md and include someip.md in the hidden toctree so the page appears in the sidebar on jumpstarter.dev. The someip.md symlink to the driver README was already created by the scaffolding script but was not wired into the docs index. Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py (1)
319-327: Consider documenting thattimeoutis intentionally ignored.The
call()method accepts atimeoutparameter to match theopensomeipAPI signature but doesn't use it since responses are returned immediately. A brief comment would clarify this is intentional for the test double.📝 Suggested documentation
def call(self, message_id, *, payload: bytes = b"", timeout: float = 5.0): - """Simulate an RPC call. Returns a canned response or echoes the payload.""" + """Simulate an RPC call. Returns a canned response or echoes the payload. + + Note: timeout is accepted for API compatibility but ignored (responses are immediate). + """ self._require_started()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py` around lines 319 - 327, The call() test double currently accepts a timeout parameter but ignores it; update the docstring or add a one-line inline comment in the call method (around the def call signature and before using _rpc_responses) stating that timeout is intentionally ignored because this fake returns immediate responses (via _rpc_responses and _FakeMessage) to match the opensomeip API signature for tests; no behaviour changes required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.py`:
- Around line 319-327: The call() test double currently accepts a timeout
parameter but ignores it; update the docstring or add a one-line inline comment
in the call method (around the def call signature and before using
_rpc_responses) stating that timeout is intentionally ignored because this fake
returns immediate responses (via _rpc_responses and _FakeMessage) to match the
opensomeip API signature for tests; no behaviour changes required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b554e280-6e16-4639-b6d0-5d00b41e8a7a
📒 Files selected for processing (3)
python/docs/source/reference/package-apis/drivers/index.mdpython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py
| requires-python = ">=3.11" | ||
| dependencies = [ | ||
| "jumpstarter", | ||
| "opensomeip>=0.1.2", |
There was a problem hiding this comment.
hey, quick one: this needs an upper version pin. right now opensomeip>=0.1.2 means a future 0.2.0 (or 1.0.0) could land and silently break things since you're relying on internal APIs like _sync_queue. something like opensomeip>=0.1.2,<0.2.0 would keep you safe.
also, have you checked the license on opensomeip? this project is Apache-2.0 so we need to make sure there's no copyleft surprise hiding in there. a quick pip show opensomeip | grep License should settle it.
[AI-generated, human-reviewed]
There was a problem hiding this comment.
Fixed the version to be on safe side. Nonetheless, I'm planning to keep opensomeip-python retro compatible. Regarding the license, there's no surprise. It's Apache-2.0 only.
| requires-python = ">=3.11" | ||
| dependencies = [ | ||
| "jumpstarter", | ||
| "opensomeip>=0.1.2", |
There was a problem hiding this comment.
why opensomeip over something like someipy or other SOME/IP Python libraries? was there a specific technical reason, or was it more about familiarity? would be good to know the trade-offs considered.
There was a problem hiding this comment.
opensomeip is the only open source stack compliant with the open-someip-spec (https://github.com/some-ip-com/open-someip-spec) to my knowledge. The other available stacks are either based on older specifications and/or even non open source specifications e.g. AUTOSAR.
|
|
||
| from pydantic import BaseModel, field_validator | ||
|
|
||
| _HEX_RE = re.compile(r"[0-9a-fA-F]*") |
There was a problem hiding this comment.
this regex has a bug: [0-9a-fA-F]* will happily accept odd-length hex strings like "ABC" (1.5 bytes), which isn't valid hex-encoded data. should be ([0-9a-fA-F]{2})* to enforce even-length (whole bytes only). worth adding a test that confirms odd-length strings get rejected.
| class SomeIpMessageResponse(BaseModel): | ||
| """A received SOME/IP message.""" | ||
|
|
||
| service_id: int |
There was a problem hiding this comment.
SOME/IP service IDs, instance IDs, method IDs, and eventgroup IDs are all 16-bit values per the spec. right now these are just int with no bounds checking, so someone could pass service_id=0x1FFFF and it'd silently overflow or produce garbage on the wire. a Field(ge=0, le=0xFFFF) constraint on these fields across SomeIpMessageResponse, SomeIpServiceEntry, and SomeIpEventNotification would catch that early.
There was a problem hiding this comment.
fixed. tests added as well.
| @validate_call(validate_return=True) | ||
| def receive_event(self, timeout: float = 5.0) -> SomeIpEventNotification: | ||
| """Receive the next event notification.""" | ||
| receiver = self._osip_client.event_subscriber.notifications() |
There was a problem hiding this comment.
isnt this the same problem as receive_message?
There was a problem hiding this comment.
Same answer as for receive_message -- event_subscriber.notifications() returns a reference to the existing receiver; it doesn't create a new object or discard the queue.
| event = threading.Event() | ||
|
|
||
| def on_found(svc: ServiceInstance) -> None: | ||
| found.append( |
There was a problem hiding this comment.
there's a subtle race here: on_found appends to found from whatever thread opensomeip's callback runs on, while the caller iterates found after event.wait() returns on the main thread. if callbacks keep firing after the first event.set(), you could get a concurrent append during iteration.
protecting found.append() with a threading.Lock and returning list(found) (a snapshot) under the lock would close that window.
[AI-generated, human-reviewed]
| @validate_call(validate_return=True) | ||
| def receive_message(self, timeout: float = 2.0) -> SomeIpMessageResponse: | ||
| """Receive a raw SOME/IP message.""" | ||
| receiver = self._osip_client.transport.receiver |
There was a problem hiding this comment.
same pattern as receive_event() below: self._osip_client.transport.receiver is accessed fresh each call. if receiver is a property that creates a new object i am not sure but do you lose queued messages between calls?
There was a problem hiding this comment.
The underlying opensomeip binding maintains the queue internally; fetching the property doesn't destroy the queue.
| self._osip_client = OsipClient(config) | ||
| self._osip_client.start() | ||
|
|
||
| def close(self): |
There was a problem hiding this comment.
if self._osip_client.stop() raises, super().close() still gets called (good!). but should close_connection() (line 216) have similar defensive handling? right now close_connection() calls stop() bare, so an exception there would propagate unhandled to the client.
[AI-generated, human-reviewed]
|
|
||
| `jumpstarter-driver-someip` provides SOME/IP (Scalable service-Oriented MiddlewarE over IP) | ||
| protocol operations for Jumpstarter. This driver wraps the | ||
| [opensomeip](https://github.com/vtz/opensomeip-python) Python binding to enable remote |
There was a problem hiding this comment.
can you double-check this URL is correct and resolves? just want to make sure it doesn't 404 after merge.
[AI-generated, human-reviewed]
- Pin opensomeip dependency to <0.2.0 in pyproject.toml to prevent breakage from future internal API changes. - Fix hex payload validation regex in common.py to strictly enforce even-length strings (whole bytes only). - Add 16-bit bounds checking (0 to 0xFFFF) to all SOME/IP ID fields in Pydantic models to prevent silent overflows. - Fix potential race condition in find_service by protecting the results list with a threading.Lock during callback execution. - Add defensive exception handling to close_connection to ensure client stop failures do not propagate unhandled.
- Add parametrized tests for odd-length hex string rejection - Add parametrized tests for 16-bit ID overflow rejection - Add close_connection exception handling survival test
2 participants
Summary
jumpstarter-driver-someippackage providing remote access to SOME/IP (Scalable service-Oriented MiddlewarE over IP) protocol operations via the opensomeip Python bindingSOMEIP_INTEGRATION_TESTS=1)Driver API
rpc_call()send_message()/receive_message()find_service()subscribe_eventgroup()/unsubscribe_eventgroup()/receive_event()close_connection()/reconnect()Test plan
make pkg-test-jumpstarter-driver-someip)make lint-fix)SOMEIP_INTEGRATION_TESTS=1)Made with Cursor