Repository navigation
Conversation
…ction 1 For implements that declare several sections that really are one, such as an RDS Artemis with three dispensers on one working width. With the setting on, every implement section follows AOG's section 1, and the heartbeat reports a single section to AOG (on while any implement section is on). Default off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nto one AOG section Booms with their own condensed DDIs (e.g. an RDS Artemis with one Function per dispenser) number their sections from 1 each. The TC kept one element per DDI, so every setpoint and DDI 160 went to the last boom, with all sections packed into it; the other booms were never put in automatic. - Resolve, per section, the nearest element carrying DDI 290 (or settable 161). With two or more such owners, each gets its own setpoint (sections numbered within it) and its own DDI 160; DDI 161 and 160 reports are read per owner; automatic while any boom is. Implements with one owner keep the existing path (checked against the 24 sample DDOPs: none has more than one). - Request each section's Y offset and width when the DDOP only declares them as process data, and log them. - groupOverlappingSections (default off): sections with the same Y offset and width (10 mm) share one AOG section; the heartbeat reports AOG's sections. The mapping is logged even while the setting is off. - section_layout.hpp holds both decisions as pure functions, with unit tests; test_section_layout also describes captured DDOP files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ns is off Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideData structuresDefine the condensed-owner and section-geometry mappings, plus the per-client state that stores owners, DDI 160 reports, and implement-to-AOG section mappings.
APIExpose the setting and server methods for applying optional overlap grouping and the client-state accessors needed by section control.
Uses of the APIApply the configured grouping to the server, remove the former follow-first overrides, and report grouped section states and mapped demand to AOG.
ImplementationResolve per-boom condensed owners, route each boom’s setpoints and DDI reports independently, and aggregate its automatic state while retaining the single-owner path.
Request missing section geometry, compute and log overlap mappings, map AOG section demand and reports, and send owner-specific setpoints and section-control commands.
ConfigurationLoad, save, expose, and default the new groupOverlappingSections setting.
DocumentationDocument the opt-in overlap-grouping setting, its geometry requirements, and the required AOG section count.
TestsTest condensed-owner resolution and overlap grouping, and provide a mode for inspecting captured DDOP layouts.
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 8 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/task_controller.cpp" line_range="735-737" />
<code_context>
+ // sections from 1 each; give every one of them its own setpoint and DDI 160.
+ if (hasCondensedSetpoint || hasSettableCondensedActual)
+ {
+ const auto firstDdi = static_cast<std::uint16_t>(hasCondensedSetpoint ? isobus::DataDescriptionIndex::SetpointCondensedWorkState1_16 : isobus::DataDescriptionIndex::ActualCondensedWorkState1_16);
+ const auto lastDdi = static_cast<std::uint16_t>(hasCondensedSetpoint ? isobus::DataDescriptionIndex::SetpointCondensedWorkState241_256 : isobus::DataDescriptionIndex::ActualCondensedWorkState241_256);
+ auto owners = section_layout::resolve_condensed_owners(state.get_pool(), element_hierarchy::build_parent_map(state.get_pool()), sectionElementNumbers, firstDdi, lastDdi);
+ if (owners.size() > 1)
+ {
</code_context>
<issue_to_address>
**Some booms miss setpoints**
When a DDOP uses DDI 290 on one boom and only settable DDI 161 on another, `hasCondensedSetpoint` selects one range for the whole pool, so `resolve_condensed_owners()` omits the boom using the other range; `owners.size()` then misses the per-boom path and some booms receive no setpoints.
Resolve owners across both condensed-DDI ranges per element rather than selecting one range for the whole pool.
</issue_to_address>
### Comment 2
<location path="include/section_layout.hpp" line_range="40-42" />
<code_context>
+ {
+ auto child = pool.get_object_by_id(childId);
+ if (child && (child->get_object_type() == ObjectTypes::DeviceProcessData))
+ {
+ const auto ddi = std::static_pointer_cast<DeviceProcessDataObject>(child)->get_ddi();
+ if ((ddi >= firstDdi) && (ddi <= lastDdi))
+ {
+ return true;
</code_context>
<issue_to_address>
**Read-only boom misses control**
When a multi-owner DDOP has a settable DDI 161 on one boom and a read-only DDI 161 on another, `resolve_condensed_owners()` treats the linked read-only DDI 161 as an owner, so `send_owner_section_setpoint_states()` sends it commands it cannot follow; that boom does not follow AOG section commands.
Only resolve a linked DDI as an owner when its process-data object is settable.
Also at `src/task_controller.cpp:1379`.
</issue_to_address>
### Comment 3
<location path="src/task_controller.cpp" line_range="373-379" />
<code_context>
+
+void ClientState::set_element_section_control_state(std::uint16_t elementNumber, bool enabled)
+{
+ elementSectionControlStates[elementNumber] = enabled;
+ bool anyEnabled = false;
+ for (const auto &entry : elementSectionControlStates)
+ {
+ anyEnabled = anyEnabled || entry.second;
+ }
+ isSectionControlEnabled = anyEnabled;
+}
+
</code_context>
<issue_to_address>
**Automatic boom stops receiving setpoints**
When one boom reports manual before another already-automatic boom's first DDI 160 report, `set_element_section_control_state()` aggregates only elements that have reported DDI 160, so it sets the overall state to false and `update_section_states` stops sending setpoints even though a boom is automatic.
Include every registered section-control element in the aggregate so an unreported element is not treated as manual.
</issue_to_address>
### Comment 4
<location path="src/task_controller.cpp" line_range="892-893" />
<code_context>
- clients[partner].set_section_control_enabled(processDataValue == 1);
+ if (clients[partner].has_multiple_condensed_owners())
+ {
+ clients[partner].set_element_section_control_state(elementNumber, processDataValue == 1);
+ }
+ else
+ {
</code_context>
<issue_to_address>
**Old replies alter the new layout**
When an old report is still in flight when the same client reactivates a pool that reuses an element number, `on_value_command()` identifies reports only by client, DDI, and element number, so a delayed DDI 160 or geometry reply from a previous activation updates the replacement `ClientState` when element numbers are reused. It can mark a boom automatic or change the new layout’s AOG grouping, and the caller sees control or section mapping based on stale data.
Associate reports with the active pool generation or otherwise discard replies from prior activations.
Also at `src/task_controller.cpp:906-911`.
</issue_to_address>
### Comment 5
<location path="src/task_controller.cpp" line_range="379" />
<code_context>
+ {
+ anyEnabled = anyEnabled || entry.second;
+ }
+ isSectionControlEnabled = anyEnabled;
+}
+
</code_context>
<issue_to_address>
**Delayed report re-enables section control**
When an older enabled DDI 160 report arrives after the element has reported disabled, `set_element_section_control_state()` overwrites the element’s remembered DDI 160 value and recomputes the aggregate with OR. A delayed pre-disable `enabled` report arriving after a newer disable report therefore makes the aggregate enabled again, and the TC continues sending section setpoints after the caller requested manual mode.
Prevent reports predating a newer control command from overwriting its state, or otherwise reconcile per-element reports with the latest commanded mode.
</issue_to_address>
### Comment 6
<location path="src/task_controller.cpp" line_range="1183" />
<code_context>
+ if (sectionStates[aogSection] != (state.get_section_setpoint_state(i) == SectionState::ON))
{
- state.set_section_setpoint_state(i, sectionStates[i] ? SectionState::ON : SectionState::OFF);
+ state.set_section_setpoint_state(i, sectionStates[aogSection] ? SectionState::ON : SectionState::OFF);
requiresUpdate = true;
+ anyUpdate = true;
</code_context>
<issue_to_address>
**Dropped boom setpoint is not retried**
When a per-owner setpoint command is not delivered while the others are, the section setpoint state is updated before `send_owner_section_setpoint_states()` sends the per-owner commands. If a send for one owner fails or is dropped, a repeated identical AOG demand produces no `anyUpdate`, so that boom remains at the old setpoint until its demand changes; the caller sees inconsistent boom control.
Track per-owner commands that still need delivery and retry them until acknowledged or otherwise confirmed.
</issue_to_address>
### Comment 7
<location path="include/section_layout.hpp" line_range="168-169" />
<code_context>
+ for (std::size_t g = 0; g < representative.size(); g++)
+ {
+ const auto &first = sections[representative[g]];
+ if ((std::abs(*first.yOffset_mm - *sections[i].yOffset_mm) <= toleranceMm) &&
+ (std::abs(*first.width_mm - *sections[i].width_mm) <= toleranceMm))
+ {
+ aogSection = static_cast<std::uint8_t>(g);
</code_context>
<issue_to_address>
**Distant sections merge**
When reported section offsets or widths have a difference outside the signed 32-bit range, or a difference is INT32_MIN, `group_overlapping_sections()` subtracts signed 32-bit geometry values before calling `std::abs`; the subtraction overflows or `std::abs` cannot represent the result, causing undefined behavior and allowing distant sections to map to the same AOG section.
Widen the geometry values before subtracting and take the absolute difference in a type that can represent it.
</issue_to_address>
### Comment 8
<location path="src/app.cpp" line_range="762-774" />
<code_context>
}
- std::vector<uint8_t> data = { state.is_section_control_enabled(), state.get_number_of_sections() };
+ // AOG's sections: overlapping implement sections are reported as one (identity mapping otherwise)
+ const std::uint8_t aogSections = state.get_number_of_aog_sections();
+ std::vector<uint8_t> data = { state.is_section_control_enabled(), aogSections };
std::uint8_t sectionIndex = 0;
- while (sectionIndex < state.get_number_of_sections())
+ while (sectionIndex < aogSections)
{
std::uint8_t byte = 0;
for (std::uint8_t i = 0; i < 8; i++)
{
- if (sectionIndex < state.get_number_of_sections())
+ if (sectionIndex < aogSections)
{
- byte |= (state.get_section_actual_state(sectionIndex) == SectionState::ON) << i;
+ byte |= state.is_aog_section_on(sectionIndex) << i;
sectionIndex++;
}
</code_context>
<issue_to_address>
**Grouped sections confuse demand diagnostics**
When `groupOverlappingSections` is enabled and PGN 229 has a set bit in an index beyond the reported AOG section count, `check_section_demand()` still compares PGN 229 demand bits by implement-section index and scans up to the implement section count, while this heartbeat reports the smaller AOG section count when grouping is applied. Padding or otherwise unused bits can therefore make the TC conclude that AOG demands work when it does not, suppressing the warning and hardware alert for an implement working without demand.
Evaluate demand only over the AOG section count (or map each implement section to its AOG demand index) before deciding whether demand exists.
</issue_to_address>
This was referenced Nov 11, 2026
…t, demand from bit 1 only PGN 229 has 64 bits, so sections 65-255 kept their old setpoint; the follow-first vector now covers every section a DDOP can declare. With several section-controlled implements the heartbeat sent one one-section report per implement. It now sends a single report: auto while any implement is in auto, on while any implement section is on. check_section_demand looked at AOG's bit for each implement section; in follow-first mode only AOG's section 1 is demand, so a stray bit can no longer hide the working-without-demand warning. Addresses Sourcery's review of #130. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…o feature/per-boom-section-control Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…g math, mapped demand - A read-only DDI 161 only reports, so it no longer makes an element a condensed owner (only applies when the pool has no DDI 290). - Booms that have not reported DDI 160 yet take the last commanded mode, so one boom reporting manual first no longer switches section control off for booms that are still automatic. - group_overlapping_sections compares in 64 bits: offsets at the ends of the int32 range could overflow and merge distant sections. - check_section_demand reads AOG's bit for the AOG section each implement section maps to, not the implement section's own index. Addresses Sourcery's review of #131. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 tasks
allSectionsFollowFirst was a quick override to validate the RDS Artemis on one width. Grouping by geometry does the same for that machine and also handles implements it cannot, such as a 3 + 3 m seeder with three products per half (two AOG sections), so the override is removed. The demand check keeps using each implement section's AOG section. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gunicsba
changed the base branch from
feature/all-sections-follow-first
to
develop
November 11, 2026 19:02
Linked issuesChecked at commit 7c02ace #115 A DDOP with two booms: the condensed setpoint goes to the second boom's element only, the first boom never moves · 1 of 1 met
|
gunicsba
changed the title
3 tasks
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.
Closes #115. Part of #132 (follow-ups: #133, #134, #136). Replaces #130:
allSectionsFollowFirstis dropped, because grouping by geometry covers it.Summary
Two changes to how the TC maps an implement's sections:
ddiToElementNumber, where the last one wins), so every setpoint and DDI 160 went to the last boom, with all sections packed into it. The other booms were never put in automatic.groupOverlappingSectionssetting, defaultfalse.Examples
developBefore this PR, the unlinked Artemis had DDI 160 and every setpoint sent to Function 3 only, as a 3-section value, and dispensers 1 and 2 were never switched to automatic.
Changes
Multi-boom support
section_layout.hpp. For each section,resolve_condensed_owners()finds the nearest element (the section itself or an ancestor) that links DDI 290, or a settable DDI 161, and groups consecutive sections by that owner.Collapsing sections
group_overlapping_sections(): sections with the same Y offset and width (within 10 mm) share an AOG section.groupOverlappingSectionson:geometry gives 1 AOG section(s) for 3 implement section(s) (1->1, 2->1, 3->1), not applied: groupOverlappingSections is off. That way its behaviour on other machines can be checked before the default changes.Tests
test_section_layoutcovers owner resolution (including read-only DDI 161) and grouping (including int32 extremes).Notes
settings.jsonstill hasallSectionsFollowFirst, the key is ignored and dropped on the next save.Testing
ctest: 8/8 pass.allSectionsFollowFirst:groupOverlappingSections:reporting 1 section(s) to AOG (1->1, 2->1, 3->1). All three Functions acknowledged automatic, and dispensers 2 and 3 followed 3 on/off cycles. Dispenser 1 has a hardware fault on that machine.🤖 Generated with Claude Code