Repository navigation
Conversation
setup_tecu_interfaces() said a secondary waits, but plan_tecu_messages() with no knowledge of the other Tractor ECUs answers "send everything". A secondary doubled the messages of the tractor for up to 2.5 s after claiming an address, which ISO 11783-9 4.4.2.1 c forbids. Start from an empty plan instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ISO 11783-7 B.1 lists Time/Date as "on request", so the TECU answers requests and never sends it unrequested. The answer is UTC from the PC clock plus the local offset, which now includes daylight saving time. It follows the TECU plan: the primary always answers, and a secondary only when no lower Tractor ECU advertises or sends Time/Date (ISO 11783-9 4.4.2.1). The facility bit in PGN 65033 follows the same state. Time/Date seen from another Tractor ECU counts for 30 s rather than 3 s, because it may only appear on request. The answer lives in TractorFacilities' PGN request handling rather than in TimeDateInterface: that registers a global receive callback, which the stack can still call once after the TECU is rebuilt. Replaces the on-request half of #87, without its 10 s broadcast. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Nov 8, 2026
Contributor
Author
|
@sourcery-ai review |
Reviewer's GuideData structuresTrack lower TECUs that have sent Time/Date until they leave or their address is reused, so secondary-provider decisions do not rely on a cyclic-message timeout.
Expose the Time/Date PGN, payload helpers, and request-handling state used to answer requests and advertise the facility only when it can be provided.
APIAdd the synchronized PGN 65254 request callback lifecycle and public helpers for encoding the Time/Date payload.
Uses of the APIInitialize and update the TECU plan so secondary startup begins empty and Time/Date request handling follows the plan's provider decision.
Clear remembered Time/Date providers when TECUs leave or change identity, mark observed providers, and use that persistent state in coexistence decisions.
ImplementationAdvertise Time/Date only when the request handler is registered, the TECU plan permits responses, and the PC clock is representable; encode the response from UTC and local clock fields.
ConfigurationDocument the concurrency guarantees for the atomic Time/Date activation flag shared between plan updates and request callbacks.
DocumentationDocument on-request Time/Date behavior, its PGN 65033 facility bit, and the revised secondary-provider rules.
TestsAdd unit coverage for persistent Time/Date provider tracking and UTC-offset and payload encoding boundaries.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/tractor_facilities.cpp" line_range="432-434" />
<code_context>
PGN_REQUIRED_TRACTOR_FACILITIES, &TractorFacilities::on_required_facilities, this);
std::cout << "[" << get_timestamp() << "] [TractorFacilities] Registered PGN 65032 diagnostic listener." << std::endl;
+ if (!pgnRequestProtocol->register_pgn_request_callback(PGN_TIME_DATE, &TractorFacilities::on_time_date_request, this))
+ {
+ std::cout << "[" << get_timestamp() << "] [TractorFacilities] Failed to register PGN 65254 request callback." << std::endl;
+ }
+
</code_context>
<issue_to_address>
**Advertised service gets no response**
When registration of the PGN 65254 request callback fails, `initialize()` logs the failure but still marks the object initialized and returns success; the application can then keep Time/Date active and advertise it in PGN 65033, although no callback answers requests.
Make initialization fail or disable the Time/Date facility when callback registration fails.
</issue_to_address>
### Comment 2
<location path="src/app.cpp" line_range="505" />
<code_context>
tractorFacilities->set_speed_messages_interface(speedMessagesInterface.get());
tractorFacilities->set_nmea2000_message_interface(nmea2000MessageInterface.get());
tractorFacilities->set_provided_facilities(facilities_for_plan(plan));
+ tractorFacilities->set_time_date_active(plan.sends(TecuMessage::TimeDate));
tractorFacilities->initialize();
tecuPlan = plan;
</code_context>
<issue_to_address>
**Advertised time service cannot respond**
When the PC clock is outside the PGN's representable range, `set_time_date_active()` follows the plan, so `build_payload()` advertises Time/Date even though `read_system_time_date()` rejects the clock and `on_time_date_request()` refuses to answer; implement clients see an unusable capability.
Advertise Time/Date only when the PC clock is representable.
Also at `src/tractor_facilities.cpp:487-489`, `src/app.cpp:640`.
</issue_to_address>
### Comment 3
<location path="src/tractor_facilities.cpp" line_range="487" />
<code_context>
+ std::array<std::uint8_t, 8> payload{};
+ // Returning false leaves it to the Tractor ECU that does provide it: the stack ignores a global
+ // request and NACKs one addressed to us
+ if ((PGN_TIME_DATE != parameterGroupNumber) || (nullptr == self) || !self->timeDateActive || !read_system_time_date(payload))
+ {
+ return false;
</code_context>
<issue_to_address>
**Global requests get duplicate replies**
When a lower-instance TECU responds to the same global request without having previously advertised or sent Time/Date, `on_time_date_request()` sends immediately based on the secondary's plan, before `OtherTecuMonitor` observes the lower-instance TECU's reply, so the implement receives both responses.
Have the secondary avoid answering a global request when a lower-instance TECU can answer it.
</issue_to_address>
### Comment 4
<location path="include/tecu_coexistence.hpp" line_range="114" />
<code_context>
/// How long a message seen from another TECU counts as "sent by it"
static constexpr std::uint32_t SENT_BY_OTHERS_WINDOW_MS = 3000;
+ /// The same for time/date, which may only appear when something requests it
+ static constexpr std::uint32_t TIME_DATE_SENT_BY_OTHERS_WINDOW_MS = 30000;
/// @brief Register the PGN 65033 listener. Call once.
</code_context>
<issue_to_address>
**Departed provider suppresses replies**
When a lower-instance TECU sends Time/Date and then leaves before its 30-second window expires, `refresh()` removes the departed TECU's facility record but leaves its timestamp in `lastSentByOthersMs`, so `sent_by_others()` continues treating Time/Date as provided and the secondary withholds replies for the remainder of the window.
Clear the departed TECU's Time/Date timestamp when `refresh()` removes it.
Also at `src/tecu_coexistence.cpp:278`.
</issue_to_address>
### Comment 5
<location path="src/tecu_coexistence.cpp" line_range="278" />
<code_context>
const std::uint32_t last = lastSentByOthersMs[i];
- sent[i] = (0 != last) && !isobus::SystemTiming::time_expired_ms(last, SENT_BY_OTHERS_WINDOW_MS);
+ const bool isTimeDate = (static_cast<std::size_t>(TecuMessage::TimeDate) == i);
+ sent[i] = (0 != last) && !isobus::SystemTiming::time_expired_ms(last, isTimeDate ? TIME_DATE_SENT_BY_OTHERS_WINDOW_MS : SENT_BY_OTHERS_WINDOW_MS);
}
return sent;
</code_context>
<issue_to_address>
**Spoofed time frames disable secondary replies**
When an attacker on the CAN bus claims a lower-instance Tractor ECU identity and sends PGN 65254, `OtherTecuMonitor` treats the frame as another TECU's time/date message, and the 30-second window suppresses `TimeDate` in the secondary's plan; `on_time_date_request` then refuses to answer, so implements lose the secondary's time/date service.
Do not let unauthenticated time/date frames alone disable the secondary's response; validate the provider against a trusted source policy before applying the suppression window.
Also at `src/tractor_facilities.cpp:487`.
</issue_to_address>
### Comment 6
<location path="src/tecu_coexistence.cpp" line_range="278" />
<code_context>
const std::uint32_t last = lastSentByOthersMs[i];
- sent[i] = (0 != last) && !isobus::SystemTiming::time_expired_ms(last, SENT_BY_OTHERS_WINDOW_MS);
+ const bool isTimeDate = (static_cast<std::size_t>(TecuMessage::TimeDate) == i);
+ sent[i] = (0 != last) && !isobus::SystemTiming::time_expired_ms(last, isTimeDate ? TIME_DATE_SENT_BY_OTHERS_WINDOW_MS : SENT_BY_OTHERS_WINDOW_MS);
}
return sent;
</code_context>
<issue_to_address>
**Recent time/date transmission gets missed**
When a TECU sends PGN 65254 while the secondary evaluates its plan, `on_frame()` writes `lastSentByOthersMs` on the CAN callback thread while `sent_by_others()` reads it on the TECU update thread without synchronization. A concurrent read and write is a data race, so the secondary can make its plan without reliably seeing a recent time/date transmission and answer a request the other TECU should handle.
Synchronize accesses to `lastSentByOthersMs`, or make its elements atomic.
</issue_to_address>Time/Date is only sent on request, so the 30 s window let a secondary answer alongside a lower TECU whenever requests came more than 30 s apart, and kept deferring for up to 30 s after that TECU left. A lower TECU now counts as providing Time/Date from the first time it sends it until it leaves the bus or another TECU takes its address (ISO 11783-9 4.4.2.1). The Time/Date facility bit in PGN 65033 is now only set while the TECU can answer: the PGN 65254 request callback registered and the PC clock is within the range the message can carry (1985-2235). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #118 (secondary TECU), because whether we may answer depends on what the other Tractor ECUs provide. Split out of #87: this is its on-request half, without the 10 s broadcast. ISO 11783-7 B.1 lists Time/Date as "on request".
What changes
0xF8.Differences from #87
mktime()comparison reported the standard-time offset in summer. The offset now comes from comparing the UTC and local clock fields directly, which also makes it unit-testable without touching the time zone.TimeDateInterface. It registers a global receive callback, and the stack can still call one once after removal, which would be a use-after-free when Run the TECU as a secondary Tractor ECU when the tractor has one #118 rebuilds the TECU. The answer uses the PGN request callback inTractorFacilities, whose removal is synchronised. Without the broadcast, nothing needs that receive callback.fee6Mutexor duplicate-provider listener. Run the TECU as a secondary Tractor ECU when the tractor has one #118'sOtherTecuMonitoralready tracks other providers.Also fixes a bug in #118 (separate commit)
A secondary started from a "send everything" plan, because
plan_tecu_messages()with no information reports every message as a gap. It doubled the tractor's messages for up to 2.5 s after claiming an address, contrary to the behaviour indocs/PROTOCOL.md§5.7. It now starts with an empty plan. Time/Date needs this too, so that we don't answer before knowing what the tractor provides. It can move into #118 if you prefer.Testing
🤖 Generated with Claude Code
Summary by Sourcery
Support standards-compliant on-request Time/Date responses while coordinating primary and secondary TECU providers and preventing premature secondary transmissions.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: