Repository navigation
PTP-P1.5: complete portable runtime and ESP control bridge - #64
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bf427aea7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void finish_runtime(PtpLabContext& context) { | ||
| g_ptp_accept_rx.store(false, std::memory_order_release); | ||
| if (context.runtime.has_value()) { |
There was a problem hiding this comment.
When ar_ptp_lab_stop() races with an incoming Pdelay request, a callback may already have passed the g_ptp_accept_rx check before this store executes; it can subsequently read context->task_handle and call xTaskNotifyGive() after finish_runtime() clears/deletes that task. The atomic gate does not wait for in-flight callbacks, so stopping or quickly restarting under Pdelay traffic can notify a stale FreeRTOS task handle or access reused context. Unregister/drain the input callback, or otherwise synchronize outstanding callbacks, before clearing the handle and deleting the task.
Useful? React with 👍 / 👎.
| if (options.vlan_id.has_value() && | ||
| (*options.vlan_id == 0U || *options.vlan_id > 4094U)) { | ||
| error = "VLAN ID must be 1..4094"; | ||
| return false; |
There was a problem hiding this comment.
For deployments using IEEE 802.1Q priority tagging, VID 0 is valid and was accepted by the previous PTP configuration, but this validation now rejects it and the matching Kconfig range starts at 1. Existing vlan_id=0 profiles therefore cannot be reconfigured and are forced onto a real VLAN such as the new default 100; reject the reserved VID 4095 while preserving VID 0.
Useful? React with 👍 / 👎.
| const std::string configured_identity{CONFIG_AR_PTP_CLOCK_IDENTITY}; | ||
| if (!configured_identity.empty()) { | ||
| ar::iec61850::time_sync::PtpClockIdentity parsed{}; | ||
| if (try_parse_ptp_clock_identity(configured_identity, parsed)) { |
There was a problem hiding this comment.
When CONFIG_AR_PTP_CLOCK_IDENTITY is nonempty but malformed, the failed parse is silently ignored, leaving clock_identity_override false and causing the firmware to transmit with a MAC-derived identity instead of reporting the configuration error. This makes a typo produce a valid but unintended on-wire clock identity; preserve the parse failure and reject the selected profile, or at minimum surface an explicit startup error.
Useful? React with 👍 / 👎.
1 participant
Summary
Completes the PTP-P1 runtime/control layer port from ARIEC61850 while preserving the stronger ESP32-P4 hardware timestamp implementation introduced in PTP-P1.
Portable runtime
PtpPublisherOptionsPtpPublisherStatusPtpSequenceCountersRuntime validation
ESP32-P4 adapter
Profile controls
Safety boundary
smpSynchpromotion; fail-safesmpSynch=0remains unchangedValidation before PR
The PR-triggered full C++ CI and final ESP current-head gates are the final merge checks.