Muse skeleton get#47
Muse skeleton get#47saksham-chawla wants to merge 3 commits intocodex/replay-20260319023913-pr-208-basefrom
Conversation
- Adds GetLatestSlot function to EventDataControlComposite Issue: SWP-249517
- Adds GetLatestSampleFunction to SkeletonEvent Issue: SWP-249517
- fixes clang format issues. Issue: SWP-249517
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
There was a problem hiding this comment.
Identified a fallback logic bug and a maintainability refactor opportunity.
Status: Changes Suggested | Risk: Medium | Readiness: 6/10 (Needs Polish)
Issues Identified & Suggestions
- Handle QM-ignore state to allow ASIL-B fallback after disconnect:
score/mw/com/impl/bindings/lola/event_data_control_composite.cpp - Refactor duplicate latest-timestamp logic using FindLatestReadableSlotIndex:
score/mw/com/impl/bindings/lola/event_data_control_composite.cpp
Review Details
📁 10 files reviewed | 💬 2 comments
👍 / 👎 individual comments to help improve reviews for you
| if (asil_b_control_ != nullptr) | ||
| { | ||
| auto& slot_qm = asil_qm_control_->state_slots_[slot_index]; | ||
| auto& slot_asil_b = asil_b_control_->state_slots_[slot_index]; | ||
|
|
||
| if (!TryIncreaseReferenceCount(slot_qm)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| if (!TryIncreaseReferenceCount(slot_asil_b)) |
There was a problem hiding this comment.
[Logic] GetLatestSlot() always tries to take a reference on the QM slot (lines 371‑387) whenever an ASIL‑B control exists. After a QM contract violation the composite sets ignore_qm_control_ = true and AllocateNextSlot()/EventReady() stop touching QM slots entirely, leaving them permanently invalid/in-writing. In that state TryIncreaseReferenceCount(slot_qm) will always fail, so this loop runs through the retry budget and returns {} even though the ASIL‑B slot for the same index is perfectly usable. As a result SkeletonEvent::GetLatestSample() starts returning ComErrc::kBindingFailure immediately after a QM disconnect, which defeats the whole point of the new API.
Please skip the QM branch when ignore_qm_control_ is true and fall back to an ASIL‑B–only indicator. For example:
if (asil_b_control_ != nullptr)
{
if (!ignore_qm_control_)
{
// existing QM+ASIL-B path
}
else
{
auto& slot_asil_b = asil_b_control_->state_slots_[slot_index];
if (!TryIncreaseReferenceCount(slot_asil_b))
{
continue;
}
return {slot_index, slot_asil_b,
ControlSlotCompositeIndicator::CompositeSlotTagType::ASIL_B};
}
}This allows GetLatestSample() to keep working after QM consumers were disconnected while still using the existing QM+ASIL logic when it is available.
Context for Agents
`GetLatestSlot()` always tries to take a reference on the QM slot (lines 371‑387) whenever an ASIL‑B control exists. After a QM contract violation the composite sets `ignore_qm_control_ = true` and `AllocateNextSlot()`/`EventReady()` stop touching QM slots entirely, leaving them permanently invalid/in-writing. In that state `TryIncreaseReferenceCount(slot_qm)` will always fail, so this loop runs through the retry budget and returns `{}` even though the ASIL‑B slot for the same index is perfectly usable. As a result `SkeletonEvent::GetLatestSample()` starts returning `ComErrc::kBindingFailure` immediately after a QM disconnect, which defeats the whole point of the new API.
Please skip the QM branch when `ignore_qm_control_` is true and fall back to an ASIL‑B–only indicator. For example:
```cpp
if (asil_b_control_ != nullptr)
{
if (!ignore_qm_control_)
{
// existing QM+ASIL-B path
}
else
{
auto& slot_asil_b = asil_b_control_->state_slots_[slot_index];
if (!TryIncreaseReferenceCount(slot_asil_b))
{
continue;
}
return {slot_index, slot_asil_b,
ControlSlotCompositeIndicator::CompositeSlotTagType::ASIL_B};
}
}
```
This allows `GetLatestSample()` to keep working after QM consumers were disconnected while still using the existing QM+ASIL logic when it is available.
File: score/mw/com/impl/bindings/lola/event_data_control_composite.cpp
Line: 381Chain of Thought
- Validation Decision: accepted
- Confidence: 0.00
- Likelihood Score: 0/5
- Suggested Action: keep
- Tool Calls Used: 6
Reasoning
Failed to parse validation response
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/event_data_control_composite.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/event_data_control_composite.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/event_data_control_composite_test.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/skeleton_event.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
5. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/control_slot_composite_indicator.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| score::cpp::optional<SlotIndexType> EventDataControlCompositeImpl<AtomicIndirectorType>::FindLatestReadableSlotIndex( | ||
| const EventDataControl& control) const noexcept | ||
| { | ||
| EventSlotStatus::EventTimeStamp latest_time_stamp{1U}; | ||
| score::cpp::optional<SlotIndexType> latest_slot_index{}; | ||
|
|
||
| for (SlotIndexType slot_index = 0U; | ||
| // Suppress "AUTOSAR C++14 A4-7-1" rule finding. This rule states: "An integer expression shall not lead | ||
| // to loss.". As the maximum number of slots is std::uint16_t, so there is no case for a data loss here. | ||
| // coverity[autosar_cpp14_a4_7_1_violation] | ||
| slot_index < static_cast<SlotIndexType>(control.state_slots_.size()); | ||
| ++slot_index) | ||
| { | ||
| SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD(static_cast<std::size_t>(slot_index) < control.state_slots_.size()); | ||
| const EventSlotStatus slot{control.state_slots_[slot_index].load(std::memory_order_acquire)}; | ||
| if (!slot.IsInvalid() && !slot.IsInWriting()) | ||
| { | ||
| const auto slot_time_stamp = slot.GetTimeStamp(); | ||
| if (latest_time_stamp < slot_time_stamp) | ||
| { | ||
| latest_time_stamp = slot_time_stamp; | ||
| latest_slot_index = slot_index; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return latest_slot_index; |
There was a problem hiding this comment.
[Maintainability] [CodeDuplication] FindLatestReadableSlotIndex is nearly identical to the existing GetLatestTimestamp method in this same file (visible at lines ~413-435 of the full file). Both iterate all slots, skip invalid/in-writing slots, and track the highest timestamp — FindLatestReadableSlotIndex additionally records the slot index. GetLatestTimestamp could be refactored to call FindLatestReadableSlotIndex internally, avoiding duplication and divergence risk.
| score::cpp::optional<SlotIndexType> EventDataControlCompositeImpl<AtomicIndirectorType>::FindLatestReadableSlotIndex( | |
| const EventDataControl& control) const noexcept | |
| { | |
| EventSlotStatus::EventTimeStamp latest_time_stamp{1U}; | |
| score::cpp::optional<SlotIndexType> latest_slot_index{}; | |
| for (SlotIndexType slot_index = 0U; | |
| // Suppress "AUTOSAR C++14 A4-7-1" rule finding. This rule states: "An integer expression shall not lead | |
| // to loss.". As the maximum number of slots is std::uint16_t, so there is no case for a data loss here. | |
| // coverity[autosar_cpp14_a4_7_1_violation] | |
| slot_index < static_cast<SlotIndexType>(control.state_slots_.size()); | |
| ++slot_index) | |
| { | |
| SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD(static_cast<std::size_t>(slot_index) < control.state_slots_.size()); | |
| const EventSlotStatus slot{control.state_slots_[slot_index].load(std::memory_order_acquire)}; | |
| if (!slot.IsInvalid() && !slot.IsInWriting()) | |
| { | |
| const auto slot_time_stamp = slot.GetTimeStamp(); | |
| if (latest_time_stamp < slot_time_stamp) | |
| { | |
| latest_time_stamp = slot_time_stamp; | |
| latest_slot_index = slot_index; | |
| } | |
| } | |
| } | |
| return latest_slot_index; | |
| // In GetLatestTimestamp: | |
| EventSlotStatus::EventTimeStamp GetLatestTimestamp() const noexcept | |
| { | |
| EventDataControl* control = (asil_b_control_ != nullptr) ? asil_b_control_ : asil_qm_control_; | |
| const auto latest_index = FindLatestReadableSlotIndex(*control); | |
| if (!latest_index.has_value()) | |
| { | |
| return EventSlotStatus::EventTimeStamp{1U}; | |
| } | |
| const EventSlotStatus slot{control->state_slots_[latest_index.value()].load(std::memory_order_acquire)}; | |
| return slot.GetTimeStamp(); | |
| } |
Context for Agents
[CodeDuplication] `FindLatestReadableSlotIndex` is nearly identical to the existing `GetLatestTimestamp` method in this same file (visible at lines ~413-435 of the full file). Both iterate all slots, skip invalid/in-writing slots, and track the highest timestamp — `FindLatestReadableSlotIndex` additionally records the slot index. `GetLatestTimestamp` could be refactored to call `FindLatestReadableSlotIndex` internally, avoiding duplication and divergence risk.
```suggestion
// In GetLatestTimestamp:
EventSlotStatus::EventTimeStamp GetLatestTimestamp() const noexcept
{
EventDataControl* control = (asil_b_control_ != nullptr) ? asil_b_control_ : asil_qm_control_;
const auto latest_index = FindLatestReadableSlotIndex(*control);
if (!latest_index.has_value())
{
return EventSlotStatus::EventTimeStamp{1U};
}
const EventSlotStatus slot{control->state_slots_[latest_index.value()].load(std::memory_order_acquire)};
return slot.GetTimeStamp();
}
```
File: score/mw/com/impl/bindings/lola/event_data_control_composite.cpp
Line: 68Chain of Thought
- Validation Decision: accepted
- Confidence: 0.90
- Likelihood Score: 4/5
- Likelihood Reason: The code duplication is clearly present and was introduced by this PR. As the codebase evolves, maintaining two nearly identical iteration loops increases the risk of divergence (e.g., one gets a bug fix the other doesn't). This is a real maintainability concern.
- Suggested Action: keep
- Tool Calls Used: 2
Reasoning
The comment accurately identifies near-identical code between the newly introduced FindLatestReadableSlotIndex (lines 41-69) and the pre-existing GetLatestTimestamp (lines 417-440 in HEAD). Both methods:
- Initialize
latest_time_stamp{1U} - Iterate over all
state_slots_of the same control - Skip slots that are invalid or in-writing with identical conditions
- Track the highest timestamp using identical comparison logic
- Use the same assertion macro and memory ordering
The only difference is that FindLatestReadableSlotIndex additionally tracks and returns the slot index. This is textbook code duplication.
The duplication is introduced by this PR — FindLatestReadableSlotIndex is entirely new code added in this diff. The suggestion to refactor GetLatestTimestamp to call FindLatestReadableSlotIndex internally is concrete and actionable.
One minor caveat with the suggested implementation: it re-reads the slot after FindLatestReadableSlotIndex returns, which could yield a different timestamp in a concurrent scenario. However, this is a minor concern since the existing code already faces similar concurrency considerations during iteration, and could be addressed by the developer during implementation (e.g., returning both index and timestamp from the helper).
The comment is accurate, actionable, and addresses a real issue introduced by this PR.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/event_data_control_composite.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (BASE)
- File:
score/mw/com/impl/bindings/lola/event_data_control_composite.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2 participants
No description provided.