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 1 commit intojumpstarter-dev:mainfrom
Conversation
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
❌ Deploy Preview for jumpstarter-docs failed. Why did it fail? →
|
📝 WalkthroughWalkthroughA complete new gPTP/PTPv2 time synchronization driver package is introduced, featuring a hardware driver that spawns and monitors ptp4l processes, a client interface for lifecycle control and status queries, data models for synchronization events, comprehensive documentation, a mock testing backend, and full test coverage including integration tests. 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
Driver->>ptp4l: generate ptp4l.conf
ptp4l->>System: sync via hardware timestamps
User->>Client: wait_for_sync(timeout=30)
loop poll every 1s until sync
Client->>Driver: is_synchronized()
Driver->>Driver: check internal state
Driver-->>Client: bool
end
User->>Client: monitor(count=5)
Client->>Driver: read()
loop async generator yields events
Driver->>ptp4l: read log output
Driver->>Driver: parse state/offset/frequency
Driver-->>Client: GptpSyncEvent
Client-->>User: display event
end
User->>Client: stop()
Client->>Driver: stop()
Driver->>ptp4l: terminate process
ptp4l->>ptp4l: cleanup
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.
| def veth_pair(self): | ||
| import subprocess as sp | ||
| cmds = [ | ||
| "ip netns add ns-ptp-master", | ||
| "ip netns add ns-ptp-slave", | ||
| "ip link add veth-m type veth peer name veth-s", | ||
| "ip link set veth-m netns ns-ptp-master", | ||
| "ip link set veth-s netns ns-ptp-slave", | ||
| "ip netns exec ns-ptp-master ip addr add 10.99.0.1/24 dev veth-m", | ||
| "ip netns exec ns-ptp-slave ip addr add 10.99.0.2/24 dev veth-s", | ||
| "ip netns exec ns-ptp-master ip link set veth-m up", | ||
| "ip netns exec ns-ptp-slave ip link set veth-s up", | ||
| ] | ||
| for cmd in cmds: | ||
| sp.run(cmd.split(), check=True) | ||
| yield ("ns-ptp-master", "veth-m", "ns-ptp-slave", "veth-s") |
There was a problem hiding this comment.
The software integration test starts the slave in the wrong namespace.
Lines 580-588 move veth-s into ns-ptp-slave, but Lines 614-618 build Gptp(interface=slave_iface) and serve it in the current process namespace. Since serve() only exposes the provided driver object and does not enter slave_ns, client.start() will spawn ptp4l -i veth-s where that interface does not exist.
Also applies to: 611-618
🤖 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 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.
| def _supports_hw_timestamping(self) -> bool: | ||
| try: | ||
| result = subprocess.run( | ||
| ["ethtool", "-T", self.interface], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| output = result.stdout | ||
| return "hardware-transmit" in output and "hardware-receive" in output |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the driver.py file to check the code structure
wc -l python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 148
🏁 Script executed:
# Read the relevant sections mentioned in the review
sed -n '160,170p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 474
🏁 Script executed:
# Read around line 222-223 to see how start() calls the method
sed -n '215,230p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 597
🏁 Script executed:
# Check the start() method signature to see if it's async
sed -n '200,250p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 1771
🏁 Script executed:
# Search for all subprocess calls in the driver
grep -n "subprocess" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 615
🏁 Script executed:
# Check imports to see what's available for async subprocess handling
head -30 python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 698
🏁 Script executed:
# Check if there are other blocking calls or patterns that might be related
grep -n "\.run\|time\.sleep\|time\.time" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 275
🏁 Script executed:
# Check the _read_ptp4l_output method to see async patterns
sed -n '280,330p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 1709
🏁 Script executed:
# Let's verify the exact line range by checking the full _supports_hw_timestamping method
sed -n '160,172p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 515
🏁 Script executed:
# Check if there are any timeout values used in the driver
grep -n "timeout\|TimeoutError" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 324
Run ethtool off the event loop.
_supports_hw_timestamping() uses blocking subprocess.run() without timeout on lines 162-169, and start() calls it directly on line 222 inside an async context. A slow or wedged ethtool will stall the driver's async RPC loop. Move this shell-out to asyncio.to_thread() or use asyncio.create_subprocess_exec() with a timeout, consistent with how the driver handles ptp4l and phc2sys processes.
🤖 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 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.
| def _require_started(self) -> None: | ||
| if self._ptp4l_proc is None: | ||
| raise RuntimeError("ptp4l not started -- call start() first") | ||
|
|
There was a problem hiding this comment.
EOF from ptp4l should invalidate the session.
When Line 183 hits EOF, _read_ptp4l_output() just exits. _require_started() on Lines 172-175 only checks for a non-None handle, so a crashed ptp4l process can still make status(), is_synchronized(), and read() report the last good state.
Also applies to: 176-200
🤖 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 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.
| self._config_file = tempfile.NamedTemporaryFile( | ||
| mode="w", suffix=".cfg", prefix="ptp4l_", delete=False | ||
| ) | ||
| self._config_file.write(config_content) | ||
| self._config_file.flush() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | head -300Repository: jumpstarter-dev/jumpstarter
Length of output: 12072
🏁 Script executed:
cat -n python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | sed -n '253,310p'Repository: jumpstarter-dev/jumpstarter
Length of output: 2424
🏁 Script executed:
cat -n python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | sed -n '200,330p'Repository: jumpstarter-dev/jumpstarter
Length of output: 5304
🏁 Script executed:
# Check if there's any try-finally or exception handling around start() at the class level
rg -A 20 "async def start" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 1526
🏁 Script executed:
# Check if there's cleanup/exception handling logic elsewhere in the file
rg -B 5 -A 5 "except|try:|finally:" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | head -80Repository: jumpstarter-dev/jumpstarter
Length of output: 1938
🏁 Script executed:
# Check how start() is called and if there's wrapping exception handling
rg -B 5 "\.start\(\)" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/Repository: jumpstarter-dev/jumpstarter
Length of output: 20612
Add try-finally to ensure cleanup on all failure paths.
The start() method lacks exception handling for partial startup failures. If an exception occurs after creating the config file (line 216) or spawning subprocesses (lines 239, 262), the instance is left in a partially initialized state. The caller must manually invoke stop() to clean up the config file and processes.
Additionally, phc2sys lacks an immediate-exit check equivalent to the one for ptp4l (lines 253–257). If phc2sys exits immediately, the failure goes undetected, leaving the instance in an inconsistent state where stop() attempts to terminate an already-dead process.
Wrap the entire startup sequence in try-finally to ensure cleanup on all exception paths, and add an immediate-exit check for phc2sys matching the pattern used for ptp4l.
🤖 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 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.
| @export | ||
| @validate_call(validate_return=True) | ||
| def get_clock_identity(self) -> str: | ||
| """Get this clock's identity string. | ||
|
|
||
| :returns: Clock identity | ||
| :rtype: str | ||
| :raises RuntimeError: If ptp4l is not started | ||
| """ | ||
| self._require_started() | ||
| return "" | ||
|
|
||
| @export | ||
| @validate_call(validate_return=True) | ||
| def get_parent_info(self) -> GptpParentInfo: | ||
| """Get information about the parent/grandmaster clock. | ||
|
|
||
| :returns: Parent and grandmaster clock information | ||
| :rtype: GptpParentInfo | ||
| :raises RuntimeError: If ptp4l is not started | ||
| """ | ||
| self._require_started() | ||
| return GptpParentInfo() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | head -100Repository: jumpstarter-dev/jumpstarter
Length of output: 3429
🏁 Script executed:
wc -l python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 148
🏁 Script executed:
cat -n python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | sed -n '360,382p'Repository: jumpstarter-dev/jumpstarter
Length of output: 927
🏁 Script executed:
# Find GptpParentInfo class definition
rg -n "class GptpParentInfo" python/packages/jumpstarter-driver-gptp/Repository: jumpstarter-dev/jumpstarter
Length of output: 180
🏁 Script executed:
# Search for linuxptp usage in the driver
rg -n "linuxptp\|ptp4l" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
sed -n '86,120p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/common.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 389
🏁 Script executed:
# Look for methods that actually query ptp4l state
rg -n "ptp4l|linuxptp|_parse|_monitor|offset|parent" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py -A 3 | head -50Repository: jumpstarter-dev/jumpstarter
Length of output: 1972
🏁 Script executed:
# Check if there's any state tracking mechanism in the driver
rg -n "self\._.*=|self\..*:" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | grep -i "offset\|parent\|identity\|clock" | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 333
🏁 Script executed:
# Check the overall structure of the driver class for state management
sed -n '100,250p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 5607
🏁 Script executed:
# Check other get_* methods in the driver to see implementation pattern
sed -n '340,500p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.py | grep -A 10 "def get_"Repository: jumpstarter-dev/jumpstarter
Length of output: 1098
🏁 Script executed:
# Search for "clock_identity" or "grandmaster" usage anywhere
rg -n "clock_identity|grandmaster|parent_clock" python/packages/jumpstarter-driver-gptp/Repository: jumpstarter-dev/jumpstarter
Length of output: 4366
🏁 Script executed:
# Check if there are any TODOs or FIXMEs in the driver
rg -n "TODO|FIXME|XXX|HACK" python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
# Look at the rest of the driver to see overall implementation
sed -n '380,591p' python/packages/jumpstarter-driver-gptp/jumpstarter_driver_gptp/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 7326
Implement get_clock_identity() and get_parent_info() to return actual values from linuxptp state.
get_clock_identity() returns an empty string and get_parent_info() returns a default GptpParentInfo() instance. These placeholder values are indistinguishable from legitimate empty/unset data, making it impossible for callers to distinguish "feature not implemented" from "no parent clock discovered yet".
The MockGptp implementation demonstrates how these should work—returning actual identity and grandmaster information. Extract these values from ptp4l output parsing or add explicit state tracking (similar to how _last_offset_ns, _port_state, and _stats are tracked), or raise an exception until the plumbing exists.
🤖 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 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.
| @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.
| @export | ||
| async def read(self) -> AsyncGenerator[GptpSyncEvent, None]: | ||
| """Stream periodic sync status updates. | ||
|
|
||
| Yields a GptpSyncEvent approximately once per second with current | ||
| offset, delay, and state information. | ||
| """ | ||
| self._require_started() | ||
| prev_state = self._port_state | ||
| for _ in range(100): | ||
| event_type = "sync" | ||
| if self._port_state != prev_state: | ||
| event_type = "state_change" | ||
| prev_state = self._port_state | ||
| if self._port_state == "FAULTY": | ||
| event_type = "fault" | ||
|
|
||
| yield GptpSyncEvent( | ||
| event_type=event_type, | ||
| port_state=PortState(self._port_state) if self._port_state in PortState.__members__ else None, | ||
| servo_state=ServoState(self._servo_state) if self._servo_state in ("s0", "s1", "s2") else None, | ||
| offset_ns=self._last_offset_ns, | ||
| path_delay_ns=self._last_path_delay_ns, | ||
| freq_ppb=self._last_freq_ppb, | ||
| timestamp=time.time(), | ||
| ) | ||
| await asyncio.sleep(1.0) |
There was a problem hiding this comment.
Both sync streams stop on an arbitrary sample cap.
The fixed range(100) on Line 419 and Line 581 forces EOF after ~100 seconds for the real driver and ~10 seconds for the mock, even if the session is healthy. That breaks long-running monitors and makes higher --count values impossible. Stream until stop/cancellation instead of a hard-coded loop bound.
Also applies to: 577-591
🤖 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 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.
| ``` | ||
| INITIALIZING → LISTENING → SLAVE (synchronized to master) | ||
| → MASTER (elected as grandmaster) | ||
| → PASSIVE (backup, not active) | ||
| → FAULTY (error detected) | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced state-machine block.
This block trips markdownlint MD040 and should declare a language (or text) to keep docs lint-clean.
🛠️ Proposed fix
-```
+```text
INITIALIZING → LISTENING → SLAVE (synchronized to master)
→ MASTER (elected as grandmaster)
→ PASSIVE (backup, not active)
→ FAULTY (error detected)</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 126-126: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @python/packages/jumpstarter-driver-gptp/README.md around lines 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 totext (or another appropriate language)
so the block becomes a labeled code fence (e.g., ```text) and the linter warning
is resolved.
</details>
<!-- fingerprinting:phantom:poseidon:hawk:219e6e3b-58b9-4c37-bc2d-22a7b6e589dd -->
<!-- This is an auto-generated comment by CodeRabbit -->
1 participant
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