Skip to content

default/config: correct five configuration annotations - #5999

Open
Moferanoluwa wants to merge 1 commit into
nasa:develfrom
Moferanoluwa:fix/config-annotation-accuracy
Open

Moferanoluwa wants to merge 1 commit into
nasa:develfrom
Moferanoluwa:fix/config-annotation-accuracy

Conversation

@Moferanoluwa

Copy link
Copy Markdown
Contributor
Related Issue(s) #5921
Has Unit Tests (y/n) n (comments/annotations only)
Documentation Included (y/n) y
Generative AI was used in this contribution (y/n) AI

Change Description

Five configuration knobs had annotations describing something other than the knob they sit on. Projects tune these from exactly these comments. Comments and annotations only — no values or behavior change.

I verified each against the code that consumes the knob rather than taking the issue at face value:

1. AcConstants.fppAssertFatalAdapterEventFileSize

The old note claimed it is "Set much smaller than FwAssertTextSize" (it's 240 vs 256, so not much smaller) and referred to "FW_LOG_STRING_MAX_SIZE (set in FW_LOG_STRING_MAX_SIZE)", which is circular. Svc::AssertFatalAdapter truncates to the minimum of this constant and FW_LOG_STRING_MAX_SIZE:

const FwSizeType outputSize = std::min(static_cast<FwSizeType>(AssertFatalAdapterEventFileSize),
                                       static_cast<FwSizeType>(FW_LOG_STRING_MAX_SIZE));

With the defaults that's min(240, 200) = 200, so FW_LOG_STRING_MAX_SIZE governs and raising this constant alone does nothing. The existing Nominal.FilePathTruncation test corroborates it — the truncated file name in its output is exactly 200 characters, not 240.

2. EventManagerCfg.hppTELEM_ID_FILTER_SIZE sizes Svc::EventManager's filtered event ID set (FwEventIdType filteredIDs[...], Fw::ArraySet<FwEventIdType, TELEM_ID_FILTER_SIZE> m_filteredIDs), not a telemetry filter. The issue offered "renamed (with deprecation alias) or documented" — I documented it, since renaming a public config constant would break existing project configs for a severity-1 annotation fix. Happy to do the rename + alias instead if you'd prefer.

3. FpConfig.fppFwTaskIdType was annotated "The type of task priorities used.", copied verbatim from FwTaskPriorityType a few lines above.

4. FpConstants.fppFW_STATEMENT_ARG_BUFFER_MAX_SIZE was annotated "the serialized telemetry value", copied verbatim from FW_TLM_BUFFER_MAX_SIZE directly above it.

5. IpCfg.hppSOCKET_SEND_TIMEOUT_MICROSECONDS was commented "Milliseconds component". The value is assigned to timeval::tv_usec in Drv/Ip/IpSocket.cpp, so the name is right and the comment was wrong.

Rationale

Fixes #5921.

Testing/Review Recommendations

No behavior to test, but the FPP annotation edits do flow into autocoded output and dictionaries, so I built and ran the affected components' unit tests: Svc_AssertFatalAdapter_ut_exe (2/2) and Svc_EventManager_ut_exe (7/7) both pass.

The AssertFatalAdapter truncation claim is worth a second look since it's the only one where I changed substance rather than just wording.

This dev machine can't natively build F´ (no Windows platform in the CMake build), so this was run in WSL Ubuntu.

Future Work

If you'd rather TELEM_ID_FILTER_SIZE were actually renamed (e.g. EVENT_ID_FILTER_SIZE with a deprecated alias), that's a small follow-up.

AI Usage (see policy)

  • Tool: Claude Code (Claude Opus)
  • Type: Verified each of the five findings against the consuming code, then corrected the annotations.
  • Scope: Five comment/annotation edits under default/config/.
  • Level of modification: I reviewed and take responsibility for every line. Each claim above was checked against the code that reads the knob, and the 200-character truncation figure comes from actual test output, not inference.

IAMAI

🤖 Generated with Claude Code

Fixes nasa#5921. Projects tune these knobs from their annotations,
and five of them described something other than the knob they sit on.
Comments/annotations only; no values or behavior change.

- AcConstants.fpp AssertFatalAdapterEventFileSize: claimed it is "set much
  smaller than FwAssertTextSize" (240 vs 256, so not much smaller) and
  pointed at "FW_LOG_STRING_MAX_SIZE (set in FW_LOG_STRING_MAX_SIZE)".
  Svc::AssertFatalAdapter truncates to min(this, FW_LOG_STRING_MAX_SIZE),
  so with the defaults FW_LOG_STRING_MAX_SIZE (200, in FpConstants.fpp)
  governs and raising this constant alone does nothing. Say that instead.
- EventManagerCfg.hpp TELEM_ID_FILTER_SIZE: sizes EventManager's filtered
  *event* ID set (FwEventIdType m_filteredIDs), not a telemetry filter.
  Documented rather than renamed, to avoid breaking existing configs.
- FpConfig.fpp FwTaskIdType: annotated "task priorities", copied from
  FwTaskPriorityType directly above it.
- FpConstants.fpp FW_STATEMENT_ARG_BUFFER_MAX_SIZE: annotated as a
  telemetry value buffer, copied from FW_TLM_BUFFER_MAX_SIZE above it.
- IpCfg.hpp SOCKET_SEND_TIMEOUT_MICROSECONDS: commented "Milliseconds";
  the value is assigned to timeval::tv_usec in IpSocket.cpp.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lestarch-autobot
lestarch-autobot self-requested a review September 18, 2026 01:53
Comment on lines +60 to +67
@ The size of a file name in an AssertFatalAdapter event (leading-truncation:
@ the tail of the path is kept)
@ Note: Svc::AssertFatalAdapter truncates to the smaller of this constant and
@ FW_LOG_STRING_MAX_SIZE (set in FpConstants.fpp), so raising this alone has no
@ effect once it exceeds that bound. With the defaults (240 here, 200 there)
@ FW_LOG_STRING_MAX_SIZE is what governs.
@ FwAssertTextSize (in this file) is a separate bound on the console assert
@ text, which also carries the timestamp and assert arguments.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Maintainability] suggestion maint-misleading-comment: eight-line annotation where three lines carry the same information.

Configuration constants are read at a glance when tuning a deployment; a paragraph-length note buries the one actionable fact (raising this alone does nothing above FW_LOG_STRING_MAX_SIZE) and will drift the next time either bound moves. Verified against AssertFatalAdapterComponentImpl.cpp:100 (std::min of the two) — the condensed form below keeps every fact that is true today.

Suggested change
@ The size of a file name in an AssertFatalAdapter event (leading-truncation:
@ the tail of the path is kept)
@ Note: Svc::AssertFatalAdapter truncates to the smaller of this constant and
@ FW_LOG_STRING_MAX_SIZE (set in FpConstants.fpp), so raising this alone has no
@ effect once it exceeds that bound. With the defaults (240 here, 200 there)
@ FW_LOG_STRING_MAX_SIZE is what governs.
@ FwAssertTextSize (in this file) is a separate bound on the console assert
@ text, which also carries the timestamp and assert arguments.
@ The size of a file name in an AssertFatalAdapter event (leading-truncation: the tail of the path is kept)
@ Svc::AssertFatalAdapter truncates to the smaller of this and FW_LOG_STRING_MAX_SIZE (FpConstants.fpp, 200 by
@ default), so raising this alone has no effect above that bound. FwAssertTextSize separately bounds the console text.

@lestarch-autobot lestarch-autobot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review summary (run 1)

Per-agent results

Agent must fix suggestion could fix future work outstanding Verdict
Security Vulnerabilities 0 0 0 0 0 Go
Supply Chain / Runner Safety 0 0 0 0 0 Go
F Prime C/C++ Design 0 0 0 0 0 Go
Documentation Currency 0 0 0 0 0 Go
Design 0 0 0 0 0 Go
Architecture 0 0 0 0 0 Go
Test Quality 0 0 0 0 0 Go
Correctness 0 0 0 0 0 Go
Operational 0 0 0 0 0 Go
Maintainability 0 1 0 0 1 Go
CI safety Go
Totals 0 1 0 0 1 Go
Supply-chain surfaces
Surface Outstanding
Dependencies clean
Vendored / submodule clean
Build / test infrastructure clean
Workflows / actions / scripts clean
Generator output clean
Prompt-injection clean
Review-system integrity clean

Merge readiness

Merge readiness: Go — all ten reviewers completed with zero outstanding must-fix items; the single open thread is a non-blocking Maintainability suggestion.


Five annotations now point at the right stars — clear for the maintainers' final approach.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

default/config: five configuration annotations describe the wrong knob (AcConstants, EventManagerCfg, FpConfig, FpConstants, IpCfg)

2 participants