feat: add gPTP driver for IEEE 802.1AS / PTP time synchronization#392
feat: add gPTP driver for IEEE 802.1AS / PTP time synchronization#392vtz wants to merge 3 commits intojumpstarter-dev:mainfrom
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 26 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new gPTP/PTPv2 time synchronization driver package including a hardware-backed driver (ptp4l/phc2sys), client API, Pydantic models, comprehensive tests (mocks and integrations), documentation, and packaging configuration. Changes
Sequence DiagramsequenceDiagram
actor User
participant Client as GptpClient
participant Driver as Gptp Driver
participant ptp4l as ptp4l Process
participant System as System Clock
User->>Client: start()
Client->>Driver: start()
Driver->>ptp4l: spawn subprocess / write config
ptp4l->>System: perform PTP sync via NIC timestamps
User->>Client: wait_for_sync(timeout=...)
loop poll
Client->>Driver: is_synchronized()
Driver-->>Client: bool
end
User->>Client: monitor(count=5)
Client->>Driver: read()
loop yields events
Driver->>ptp4l: read log output
Driver->>Driver: parse -> update state
Driver-->>Client: GptpSyncEvent
end
User->>Client: stop()
Client->>Driver: stop()
Driver->>ptp4l: terminate process
Driver->>Driver: cancel background tasks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 9
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-gptp/README.md (1)
214-219: Normalize command examples to satisfy markdownlint MD014.For command-only examples without output, remove
$prompts (or add output lines) to silence MD014 warnings.🛠️ Proposed fix
-$ j gptp start # Start PTP synchronization -$ j gptp stop # Stop PTP synchronization -$ j gptp status # Show sync status -$ j gptp offset # Show current clock offset -$ j gptp monitor -n 20 # Monitor 20 sync events -$ j gptp set-priority 0 # Force grandmaster role +j gptp start # Start PTP synchronization +j gptp stop # Stop PTP synchronization +j gptp status # Show sync status +j gptp offset # Show current clock offset +j gptp monitor -n 20 # Monitor 20 sync events +j gptp set-priority 0 # Force grandmaster role🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-gptp/README.md` around lines 214 - 219, The README command examples use shell prompt characters which triggers markdownlint MD014; update the example block so command-only lines do not include the "$" prompt (or alternatively add expected output lines), e.g., replace entries like "$ j gptp start" with "j gptp start" for each command (start, stop, status, offset, monitor -n 20, set-priority 0) to normalize the code block for MD014 compliance.
🤖 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-gptp/jumpstarter_driver_gptp/client.py`:
- Around line 113-129: wait_for_sync currently swallows all exceptions and
doesn't enforce an offset convergence threshold; change its signature to accept
an optional max_offset (seconds) and in the loop require both
self.is_synchronized() and that the current offset (e.g. obtained via
self.get_offset() or self.offset_reading method) is <= max_offset before
returning True; replace the blanket "except Exception" with catching only
expected transient errors (or let unexpected exceptions propagate) so
driver/transport failures surface instead of being silently retried, and ensure
the function returns False on timeout if convergence criteria aren't met.
In
`@python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver_test.py`:
- Around line 577-592: The test's veth_pair fixture moves veth-s into namespace
"ns-ptp-slave" but the test then constructs Gptp(interface=slave_iface) and
calls serve() in the current namespace so client.start() spawns ptp4l for veth-s
in the wrong namespace; fix by either creating/serving the slave driver inside
the slave netns or keep veth-s in the current namespace. Concretely, update the
test around veth_pair / Gptp(interface=slave_iface) / serve() so that the object
passed to serve() is created and served from inside ns-ptp-slave (use the same
netns context as veth_pair) or change veth_pair to not move veth-s out of the
test process namespace so ptp4l -i veth-s runs where the interface exists.
In `@python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py`:
- Around line 410-436: The read() async generator currently uses a fixed for _
in range(100) loop which forcibly ends the stream after ~100 samples; change
that to an indefinite loop (e.g., while True) so the generator yields until the
session is stopped/cancelled, preserving the existing logic that checks
self._port_state, prev_state, and yields GptpSyncEvent with
port_state/servo_state/offset/path_delay/freq/timestamp, and rely on task
cancellation to terminate; apply the same replacement of the fixed-range loop
with an endless loop in the corresponding mock/alternate stream implementation
(the other method using the same range-based pattern) so both real and mock
drivers stream until stopped.
- Around line 172-175: The code currently treats a non-None _ptp4l_proc as proof
ptp4l is alive; change this so an EOF/exit actually invalidates the session: in
_read_ptp4l_output detect EOF/termination and set self._ptp4l_proc = None (and
any state flags like self._synchronized = False or clear last_state buffer) so
the session is marked dead, and ensure any cleanup/notification happens there;
update _require_started to check that _ptp4l_proc is running (e.g.,
self._ptp4l_proc.poll() is None) rather than just non-None so status(),
is_synchronized(), and read() will raise once the process has exited; ensure
status/is_synchronized/read rely on that running check or the invalidated state.
- Around line 360-382: get_clock_identity() and get_parent_info() currently
return placeholders; instead populate and return real values from linuxptp state
(or raise until available). Add state fields (e.g. self._clock_identity: str and
self._parent_info: GptpParentInfo) and update them when parsing ptp4l output
(the same parsing routine that updates _last_offset_ns, _port_state, and
_stats); then have get_clock_identity() return self._clock_identity (or raise
RuntimeError if not yet known) and have get_parent_info() return
self._parent_info (or raise RuntimeError if not yet known). Ensure
_require_started() remains called and mirror MockGptp behavior for value
semantics so callers can distinguish "not implemented/unset" from valid empty
results.
- Around line 384-397: set_priority1 currently only stores _priority1 and never
applies it to ptp4l because _generate_ptp4l_config hardcodes priority1=0; update
the real driver so set_priority1 updates the running ptp4l config: have
_generate_ptp4l_config read self._priority1 (instead of hardcoding 0) and ensure
set_priority1 triggers writing the new config and reloading/restarting ptp4l
(call the existing ptp4l restart/reload routine or implement one) so the
production behavior matches the mock behavior; touch symbols: set_priority1,
_priority1, _generate_ptp4l_config, and the ptp4l start/reload method.
- Around line 160-168: The blocking call in _supports_hw_timestamping() uses
subprocess.run() and is invoked from start() inside async code; make it
non-blocking by either converting _supports_hw_timestamping to async and using
asyncio.create_subprocess_exec() with a timeout (and read stdout/stderr) or keep
it sync but call it via await asyncio.to_thread(self._supports_hw_timestamping)
from start(); ensure you preserve the stdout parsing
("hardware-transmit"/"hardware-receive"), add timeout and exception handling so
failures return False, and align behavior with how ptp4l/phc2sys are started and
managed.
- Around line 216-220: The start() method must ensure cleanup on any failure:
wrap the entire startup sequence (from creation of self._config_file through
spawning subprocesses for ptp4l and phc2sys) in a try/finally so that the
finally block calls the instance cleanup (e.g., invoking stop() or explicit
teardown of self._config_file and any started processes) to remove the temp
config and terminate any partially-started subprocesses; additionally, after
spawning phc2sys (same place where ptp4l has an immediate-exit check), add the
same immediate-exit check used for ptp4l (short sleep then poll() and
raise/cleanup if it exited) so a dead phc2sys is detected and triggers the
cleanup path.
In `@python/packages/jumpstarter-driver-gptp/README.md`:
- Around line 126-131: The fenced state-machine block starting with
"INITIALIZING → LISTENING → SLAVE (synchronized to master)" lacks a language tag
and triggers markdownlint MD040; update the opening fence from ``` to ```text
(or another appropriate language) so the block becomes a labeled code fence
(e.g., ```text) and the linter warning is resolved.
---
Nitpick comments:
In `@python/packages/jumpstarter-driver-gptp/README.md`:
- Around line 214-219: The README command examples use shell prompt characters
which triggers markdownlint MD014; update the example block so command-only
lines do not include the "$" prompt (or alternatively add expected output
lines), e.g., replace entries like "$ j gptp start" with "j gptp start" for each
command (start, stop, status, offset, monitor -n 20, set-priority 0) to
normalize the code block for MD014 compliance.
🪄 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: 459f2d69-f987-4b81-93a2-4500f1d9b823
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
python/docs/source/reference/package-apis/drivers/gptp.mdpython/docs/source/reference/package-apis/drivers/index.mdpython/packages/jumpstarter-driver-gptp/.gitignorepython/packages/jumpstarter-driver-gptp/README.mdpython/packages/jumpstarter-driver-gptp/examples/exporter.yamlpython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/__init__.pypython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/client.pypython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/common.pypython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/conftest.pypython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pypython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver_test.pypython/packages/jumpstarter-driver-gptp/pyproject.tomlpython/pyproject.toml
| def wait_for_sync(self, timeout: float = 30.0, poll_interval: float = 1.0) -> bool: | ||
| """Block until PTP synchronization is achieved or timeout expires. | ||
|
|
||
| :param timeout: Maximum time to wait in seconds | ||
| :param poll_interval: Polling interval in seconds | ||
| :returns: True if synchronized, False if timeout expired | ||
| :rtype: bool | ||
| """ | ||
| deadline = time.monotonic() + timeout | ||
| while time.monotonic() < deadline: | ||
| try: | ||
| if self.is_synchronized(): | ||
| return True | ||
| except Exception: | ||
| pass | ||
| time.sleep(poll_interval) | ||
| return False |
There was a problem hiding this comment.
wait_for_sync() can't enforce convergence and hides real failures.
The helper never checks an offset threshold, and the blanket except Exception on Lines 123-127 turns transport/driver failures into a silent retry. That makes "driver broken" indistinguishable from "still syncing" and returns True as soon as the servo locks, even if the clock is still outside the caller's acceptable offset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/client.py`
around lines 113 - 129, wait_for_sync currently swallows all exceptions and
doesn't enforce an offset convergence threshold; change its signature to accept
an optional max_offset (seconds) and in the loop require both
self.is_synchronized() and that the current offset (e.g. obtained via
self.get_offset() or self.offset_reading method) is <= max_offset before
returning True; replace the blanket "except Exception" with catching only
expected transient errors (or let unexpected exceptions propagate) so
driver/transport failures surface instead of being silently retried, and ensure
the function returns False on timeout if convergence criteria aren't met.
python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver_test.py
Outdated
Show resolved
Hide resolved
python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py
Outdated
Show resolved
Hide resolved
python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py
Show resolved
Hide resolved
python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py
Outdated
Show resolved
Hide resolved
python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py
Outdated
Show resolved
Hide resolved
| @export | ||
| @validate_call(validate_return=True) | ||
| def set_priority1(self, priority: int) -> None: | ||
| """Set clock priority1 to influence BMCA master election. | ||
|
|
||
| Lower values make this clock more likely to become grandmaster. | ||
|
|
||
| :param priority: Priority1 value (0-255) | ||
| :raises RuntimeError: If ptp4l is not started | ||
| """ | ||
| self._require_started() | ||
| self._priority1 = priority | ||
| self.logger.info("Set priority1 to %d", priority) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the driver.py file
find . -path "*/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py" -type fRepository: jumpstarter-dev/jumpstarter
Length of output: 146
🏁 Script executed:
# Check the structure of the project
fd -t f "driver.py" | grep gptpRepository: jumpstarter-dev/jumpstarter
Length of output: 144
🏁 Script executed:
# Check file size and read the driver.py file
wc -l python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 148
🏁 Script executed:
# Read lines around the set_priority1 method (lines 384-397)
sed -n '380,400p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | cat -nRepository: jumpstarter-dev/jumpstarter
Length of output: 872
🏁 Script executed:
# Read lines around 469-473 for the mock behavior
sed -n '465,480p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | cat -nRepository: jumpstarter-dev/jumpstarter
Length of output: 765
🏁 Script executed:
# Now let's check the _generate_ptp4l_config() method to see if it ignores _priority1
rg "_generate_ptp4l_config" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py -A 30Repository: jumpstarter-dev/jumpstarter
Length of output: 2005
🏁 Script executed:
# Check if _priority1 is used in _generate_ptp4l_config or elsewhere in the real driver
rg "_priority1" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 525
🏁 Script executed:
# Look at the conftest.py file to see the StatefulPtp4l setup
cat -n python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/conftest.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 4394
🏁 Script executed:
# Verify the actual lines 469-473 in driver.py to see the exact MockGptpBackend implementation
sed -n '469,473p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | cat -nRepository: jumpstarter-dev/jumpstarter
Length of output: 340
Real and mock set_priority1() define different contracts—the real driver ignores priority changes.
The real implementation (lines 384-397) only stashes priority in memory without propagating it to ptp4l. The configuration is generated once at startup via _generate_ptp4l_config(), which hardcodes priority1=0 and never reads _priority1. In contrast, the mock versions at lines 469-473 and in conftest.py perform state transitions when priority < 128, allowing tests to pass against behavior the production backend cannot provide. This violates the testing guideline that mocks should only cover system dependencies and services, not simulate unimplemented features.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py`
around lines 384 - 397, set_priority1 currently only stores _priority1 and never
applies it to ptp4l because _generate_ptp4l_config hardcodes priority1=0; update
the real driver so set_priority1 updates the running ptp4l config: have
_generate_ptp4l_config read self._priority1 (instead of hardcoding 0) and ensure
set_priority1 triggers writing the new config and reloading/restarting ptp4l
(call the existing ptp4l restart/reload routine or implement one) so the
production behavior matches the mock behavior; touch symbols: set_priority1,
_priority1, _generate_ptp4l_config, and the ptp4l start/reload method.
python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py
Show resolved
Hide resolved
| lines.append("priority1\t\t0") | ||
| lines.append("priority2\t\t0") | ||
|
|
||
| lines.append(f"\n[{interface}]") |
There was a problem hiding this comment.
The interface value gets interpolated straight into an INI section header with no validation. A config value like eth0]\nmalicious_key\tvalue\n[fake would break out of the section and inject arbitrary ptp4l configuration.
Please add a regex check in __post_init__ to restrict interface to valid Linux interface names, something like re.fullmatch(r'[a-zA-Z0-9][a-zA-Z0-9._-]{0,14}', self.interface). Same value also flows into ethtool and ptp4l subprocess args (lines 162, 236).
| "-i", self.interface, | ||
| ts_flag, | ||
| "-m", | ||
| *self.ptp4l_extra_args, |
There was a problem hiding this comment.
These extra args are splatted directly into the ptp4l exec call with zero filtering, and ptp4l runs with CAP_NET_RAW. An exporter config could pass something like ["-f", "/etc/shadow"] to override the config file, or ["--uds_address", "/tmp/evil"] to redirect the management socket.
Please add a denylist validation in __post_init__ that rejects at least -f, --config, -i, --interface, --uds_address, and --log_file. A comment explaining that ptp4l_extra_args is trusted-operator-only input would also be good.
| role=self.role, | ||
| ) | ||
|
|
||
| self._config_file = tempfile.NamedTemporaryFile( |
There was a problem hiding this comment.
This temp file is created with delete=False but only cleaned up in stop() (line 297). If the process crashes or start() raises after this point (e.g. ptp4l exits immediately at line 232), the temp file leaks. The default permissions also depend on umask, which could leave the config world-readable.
Consider wrapping the rest of start() in a try/except that calls cleanup on failure. Also worth setting mode=0o600 on the file descriptor after creation via os.fchmod.
| self._servo_state = "s0" | ||
| self._priority1 = 128 | ||
| self._stats = {} | ||
| self._reader_task = asyncio.get_event_loop().create_task( |
There was a problem hiding this comment.
asyncio.get_event_loop() is deprecated since Python 3.10 and will emit a DeprecationWarning if there's no running loop. Since this code runs inside an async method, just use asyncio.create_task(...) directly.
Also, if _read_ptp4l_output raises an unexpected exception, it'll be silently swallowed (nobody awaits this task or checks task.exception()). Consider adding a task.add_done_callback(...) that logs unhandled exceptions.
| self._require_started() | ||
|
|
||
| if self._reader_task is not None: | ||
| self._reader_task.cancel() |
There was a problem hiding this comment.
The teardown order here is inverted: the reader task (which reads from ptp4l's stdout) gets cancelled before ptp4l is terminated. This means the reader might miss the final output, or worse, the cancel could race with ptp4l's shutdown.
The safer order is: (1) terminate ptp4l, (2) wait for it with timeout, (3) kill if needed + await proc.wait(), (4) then cancel the reader task, (5) terminate phc2sys similarly, (6) clean up the config file.
| """ | ||
| self._require_started() | ||
| prev_state = self._port_state | ||
| for _ in range(100): |
There was a problem hiding this comment.
Hardcoded range(100) means the monitor stream silently stops after 100 events with no indication to the client. For a real PTP sync monitor this could mean losing visibility mid-test.
Consider while True with a way to break (e.g. checking if the process is still running), or at least make the limit configurable.
| try: | ||
| if self.is_synchronized(): | ||
| return True | ||
| except Exception: |
There was a problem hiding this comment.
Bare except Exception: pass here swallows everything, including unexpected errors like PermissionError, MemoryError, KeyboardInterrupt (well, that one is BaseException). If is_synchronized() is failing for a reason other than "not ready yet", this loop will silently retry until timeout with no indication of what's wrong.
Please narrow this to catch only the expected exceptions (like RuntimeError from the "not started" check, or the gRPC DriverError).
|
|
||
| [project.entry-points."jumpstarter.drivers"] | ||
| Gptp = "jumpstarter_driver_gptp.driver:Gptp" | ||
| MockGptp = "jumpstarter_driver_gptp.driver:MockGptp" |
There was a problem hiding this comment.
Registering MockGptp as a production entry point means it shows up in driver discovery alongside real drivers. No other Jumpstarter driver registers its mocks this way. If someone typos a config or an automation tool picks up available drivers, they could end up running the mock in production and get fake PTP data with no indication that sync isn't real.
Please remove this line. The mock is importable directly for tests; it doesn't need an entry point.
There was a problem hiding this comment.
Just an additional note, the jumpstarter-driver-power does register its MockPower driver in this way, but that is because the power driver is really just a stub that is supposed to be implemented for a specific piece of hardware. I would concur with @raballew that we should remove this and not register it similar to how the jumpstarter-driver-opendal has a mock driver, but doesn't register it.
| log_cli = true | ||
| log_cli_level = "INFO" | ||
| testpaths = ["jumpstarter_driver_gptp"] | ||
| asyncio_mode = "auto" |
There was a problem hiding this comment.
asyncio_mode = "auto" is set here but pytest-asyncio isn't listed in the dev dependencies (lines 42-45). This likely works because it's pulled in transitively from the workspace, but it's fragile and will break if someone runs this package's tests in isolation. Also the project uses anyio instead of asyncio. Please replace asyncio with anyio and fix imports.
| ) | ||
|
|
||
| if self.sync_system_clock and hw_ts: | ||
| phc2sys_cmd = ["phc2sys", "-a", "-rr", "-m"] |
There was a problem hiding this comment.
The -a flag on phc2sys makes it discover and sync all PHC-to-system-clock pairs on the host, not just the one for the managed interface. On a multi-NIC box this could have unintended side effects.
Also, start_new_session=True should be added to both create_subprocess_exec calls (here and the ptp4l one at line 225) so that the child processes don't share the driver's process group. Without this, a stray signal to the driver's process group could leave orphaned privileged ptp4l/phc2sys processes.
Add jumpstarter-driver-gptp wrapping linuxptp (ptp4l/phc2sys) to provide precision time synchronization for automotive Ethernet testing. Includes: - Gptp driver with ptp4l/phc2sys process management and log parsing - MockGptp driver for testing without PTP hardware - GptpClient with wait_for_sync helper and Click CLI - Pydantic models for status, offsets, sync events, port stats - Comprehensive test suite (unit, e2e mocked, stateful) - Documentation integrated into jumpstarter.dev docs site Made-with: Cursor
Security: - Validate interface name with regex (reject injection attacks) - Add denylist for ptp4l_extra_args (-f, --config, -i, --uds_address, etc.) - Set temp config file permissions to 0o600 via os.fchmod Async correctness: - Convert _supports_hw_timestamping to async with asyncio.create_subprocess_exec and 10s timeout (was blocking subprocess.run in async context) - Replace deprecated asyncio.get_event_loop().create_task with asyncio.create_task - Add done_callback to reader task for unhandled exception logging Process lifecycle: - Wrap start() in try/except with _cleanup() on any failure path - Add phc2sys immediate-exit check (matching ptp4l pattern) - Fix teardown order: terminate ptp4l first, then cancel reader, then phc2sys - Add await proc.wait() after kill() to prevent zombie processes - Invalidate session on ptp4l EOF (_ptp4l_proc = None, reset state) - Check returncode in _require_started() to detect exited processes phc2sys: - Replace -a (all PHCs) with -s <interface> -c CLOCK_REALTIME -w (scoped) - Use stdout=DEVNULL instead of PIPE (prevent pipe buffer deadlock) - Add start_new_session=True to both ptp4l and phc2sys API changes: - get_clock_identity, get_parent_info, set_priority1 now raise NotImplementedError on real driver (require UDS integration) - MockGptp still implements these for testing - Remove MockGptp from entry points (not a production driver) - read() streams indefinitely (while True) instead of range(100) Client: - Narrow wait_for_sync except to RuntimeError only Tests: - Update HW timestamping tests for async _supports_hw_timestamping - Add interface name validation tests (injection, too-long, valid names) - Add extra_args denylist tests - Add config generation priority1 test - Fix veth_pair fixture: keep both interfaces in root namespace - Use pytest-asyncio with asyncio_mode=auto (project convention) Documentation: - Add comprehensive docstrings to all public APIs (~80% coverage) - Add text language tag to state-machine code fence (MD040) - Remove $ prompts from CLI examples (MD014) - Document NotImplementedError for stub methods in README - Fix source_archive URL in pyproject.toml Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/conftest.py (1)
148-152: Redundant condition check inset_priority1.The inner check
if self._port_state != "MASTER"on line 150 is always true when the outer condition on line 149 is satisfied, since the outer condition already restricts_port_stateto("SLAVE", "LISTENING", "PASSIVE").🔧 Suggested simplification
def set_priority1(self, value: int): self.require_started() self._priority1 = value if value < 128 and self._port_state in ("SLAVE", "LISTENING", "PASSIVE"): - if self._port_state != "MASTER": - self._transition_to("MASTER") + self._transition_to("MASTER") self._call_log.append(f"set_priority1({value})")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/conftest.py` around lines 148 - 152, In set_priority1, remove the redundant inner check "if self._port_state != 'MASTER'" because the outer condition already guarantees _port_state is one of "SLAVE", "LISTENING", "PASSIVE"; instead directly call self._transition_to("MASTER") when value < 128 and self._port_state in ("SLAVE","LISTENING","PASSIVE"), preserving assignment to self._priority1 and the self._call_log.append("set_priority1({value})") behavior and keep references to _priority1, _port_state, _transition_to, and _call_log intact.python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver_test.py (1)
155-184: Fragile test setup bypassing__post_init__.Using
Gptp.__new__(Gptp)to directly instantiate without running__post_init__(lines 162, 172, 182) works for now but is fragile—if_supports_hw_timestampinglater depends on other initialized attributes, these tests will fail with confusing errors. Consider creating a minimal valid instance instead.🔧 Alternative approach using minimal valid instance
async def test_detect_hw_timestamping(self): mock_proc = AsyncMock() mock_proc.communicate.return_value = ( b"Capabilities:\n hardware-transmit\n hardware-receive\n hardware-raw-clock\n", b"", ) with patch("jumpstarter_driver_gptp.driver.asyncio.create_subprocess_exec", return_value=mock_proc): - driver = Gptp.__new__(Gptp) - driver.interface = "eth0" + driver = Gptp(interface="eth0") assert await driver._supports_hw_timestamping() is True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver_test.py` around lines 155 - 184, The tests currently bypass Gptp.__post_init__ by using Gptp.__new__(Gptp) before calling _supports_hw_timestamping (in tests test_detect_hw_timestamping, test_detect_sw_only_timestamping, test_detect_timestamping_ethtool_missing), which is fragile; instead create a minimal valid instance that runs initialization: either construct via the normal constructor (e.g., Gptp(...) with the minimal required args such as interface="eth0") or call the real initializer on the instance (driver.__init__(...) or driver.__post_init__()) after __new__ so that required attributes are set; if the real constructor has heavy side effects, add a lightweight test-only factory or patch side-effecting calls during construction to allow creating a properly initialized Gptp before calling _supports_hw_timestamping.
🤖 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-gptp/jumpstarter_driver_gptp/conftest.py`:
- Around line 148-152: In set_priority1, remove the redundant inner check "if
self._port_state != 'MASTER'" because the outer condition already guarantees
_port_state is one of "SLAVE", "LISTENING", "PASSIVE"; instead directly call
self._transition_to("MASTER") when value < 128 and self._port_state in
("SLAVE","LISTENING","PASSIVE"), preserving assignment to self._priority1 and
the self._call_log.append("set_priority1({value})") behavior and keep references
to _priority1, _port_state, _transition_to, and _call_log intact.
In
`@python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver_test.py`:
- Around line 155-184: The tests currently bypass Gptp.__post_init__ by using
Gptp.__new__(Gptp) before calling _supports_hw_timestamping (in tests
test_detect_hw_timestamping, test_detect_sw_only_timestamping,
test_detect_timestamping_ethtool_missing), which is fragile; instead create a
minimal valid instance that runs initialization: either construct via the normal
constructor (e.g., Gptp(...) with the minimal required args such as
interface="eth0") or call the real initializer on the instance
(driver.__init__(...) or driver.__post_init__()) after __new__ so that required
attributes are set; if the real constructor has heavy side effects, add a
lightweight test-only factory or patch side-effecting calls during construction
to allow creating a properly initialized Gptp before calling
_supports_hw_timestamping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21219289-579f-46a5-a78a-fa5b6337062c
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
python/docs/source/reference/package-apis/drivers/gptp.mdpython/docs/source/reference/package-apis/drivers/index.mdpython/packages/jumpstarter-driver-gptp/.gitignorepython/packages/jumpstarter-driver-gptp/README.mdpython/packages/jumpstarter-driver-gptp/examples/exporter.yamlpython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/__init__.pypython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/client.pypython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/common.pypython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/conftest.pypython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pypython/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver_test.pypython/packages/jumpstarter-driver-gptp/pyproject.tomlpython/pyproject.toml
✅ Files skipped from review due to trivial changes (7)
- python/packages/jumpstarter-driver-gptp/.gitignore
- python/docs/source/reference/package-apis/drivers/gptp.md
- python/pyproject.toml
- python/packages/jumpstarter-driver-gptp/examples/exporter.yaml
- python/packages/jumpstarter-driver-gptp/pyproject.toml
- python/docs/source/reference/package-apis/drivers/index.md
- python/packages/jumpstarter-driver-gptp/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/client.py
- python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/common.py
- Remove redundant inner condition in StatefulPtp4l.set_priority1 - Use Gptp(interface="eth0") instead of Gptp.__new__ in HW timestamping tests Made-with: Cursor
3 participants
Summary
jumpstarter-driver-gptpwrapping linuxptp (ptp4l/phc2sys) to provide IEEE 802.1AS / PTP precision time synchronization for automotive Ethernet testingGptpdriver (real hardware),MockGptpdriver (testing),GptpClientwithwait_for_synchelper and Click CLIserve(), and stateful tests enforcing PTP state machine transitionsDetails
Driver Architecture
Gptpdriver: Managesptp4landphc2syssubprocess lifecycle, parses real-time log output for sync status, offset measurements, port state changes, and clock quality metricsMockGptpdriver: Pluggable backend for testing without PTP hardware — supports aStatefulPtp4lbackend that enforces valid PTP state machine transitionsGptpClient: Async client withwait_for_sync(threshold_ns, timeout)convenience method and full Click CLI (j gptp status,j gptp start,j gptp offset, etc.)Models (
common.py)GptpStatus,GptpOffset,GptpSyncEvent,GptpPortStats,GptpParentInfoPortStateandServoStateenums withVALID_PORT_TRANSITIONSmapTest Coverage
serve(), error paths, CLI invocationStatefulPtp4lmock enforcing PTP state transitions, illegal transition rejection, operation orderingTest plan
make pkg-test-jumpstarter-driver-gptppasses all testsmake lintpasses with no new errorsMade with Cursor