fix: build the FreeRTOS pack at any tick rate - #756
Conversation
The sysUpTime adapter carried a static assert requiring a 64-bit TickType_t, or a 32-bit one whose configTICK_RATE_HZ divides 100. The umbrella target compiles that translation unit unconditionally, so an ordinary 1000 Hz configuration cost the integrator the whole pack, the mutex included. Remove the assert. The adapter builds everywhere and its header states where the value wraps early; #755 tracks removing the limit itself.
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR removes the FreeRTOS sysUpTime compile-time restriction, documents supported tick configurations and early rollover behaviour, clarifies the RFC 3418 uptime contract, and updates the related MISRA suppression. ChangesFreeRTOS sysUpTime behaviour
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/platforms/freertos/index.md`:
- Around line 40-45: The FreeRTOS uptime documentation incorrectly generalizes
the 50-day rollover and misstates issue `#755`. Update the warning in the uptime
conversion section to explain that non-dividing rates lose phase continuity when
a 32-bit TickType_t rolls over, with roughly 50 days applying specifically to
1000 Hz and other rates having different periods; describe `#755` as tracking
support for arbitrary tick-rate conversion rather than removal of the
compile-time limit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b63e6b26-8f06-4211-86e4-e4469e97f00d
📒 Files selected for processing (5)
Core/Interface/SolidSyslogMetaSd.hPlatform/FreeRtos/Interface/SolidSyslogFreeRtosSysUpTime.hPlatform/FreeRtos/Source/SolidSyslogFreeRtosSysUpTime.cdocs/platforms/freertos/index.mdmisra_suppressions.txt
|
@coderabbitai pause |
✅ Action performedReviews paused. |
☀️ Quality Summary Created by Quality Monitor v4.15.0 (#82d77af). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
Closes #725.
Platform/FreeRtos/CMakeLists.txtcarriesSolidSyslogFreeRtosSysUpTime.cinthe
SolidSyslogFreeRtosINTERFACE library, so linkingSolidSyslog::FreeRtoscompiles that translation unit whether or not the integrator uses the callback.
Its static assert required a 64-bit
TickType_t, or a 32-bit one whoseconfigTICK_RATE_HZdivides 100 — so a 32-bit target at FreeRTOS's own 1000 Hzdefault failed to compile the whole pack, the mutex with it. Nothing we build
sees this: every
FreeRTOSConfig.hin the repository is 100 Hz.Change Description
The issue proposed gating the translation unit on the tick configuration, the
way the lwIP DNS resolver gates on
LWIP_DNS. That cannot be written: thecondition has to hold in the preprocessor, and every FreeRTOS config — ours and
the form FreeRTOS's own demos use — defines the rate as a cast,
#define configTICK_RATE_HZ ((TickType_t) 100).#if (100 % configTICK_RATE_HZ) == 0is a hard error on that, not a false, so the gate would break exactly theconfigurations it exists to protect. Only the tick width is preprocessor-safe.
So the assert goes and the limit is documented instead. The adapter builds in
every configuration, and the two places an integrator looks say where the value
wraps early and what to do about it:
explicitly and asking for an integrator-supplied
SolidSyslogSysUpTimeFunction.SolidSyslogSysUpTimeFunctioninSolidSyslogMetaSd.hgains the property theadapter actually trips over — the wrap must reach 2^32 hundredths, so a source
counter that rolls over sooner has to carry the phase across. It gave units and
the fact of wrapping but not that, so someone writing their own implementation
had nothing to check against. That doc comment is the contract's home; the
platform page and the adapter header link it rather than restating it.
1000 Hz is the default rate, so shipping a documented limit there is not the
end of it. #755 is raised against 0.2.0 to make the conversion correct at any
tick rate, and both the header note and the platform-page warning name it.
Test Evidence
No behaviour changed: the conversion is untouched and only a compile-time
assert was removed, so the existing tests are the coverage. All 22 test
executables pass on the
freertos-hostlane,SolidSyslogFreeRtosSysUpTimeTestamong them (4 tests, 4 checks).
misra_suppressions.txtline-drift for theremoved assert was corrected by hand.
The issue asked for the gate to be proved by building in an unsupported
configuration. With no gate left there is nothing to prove that way, and adding
a second tick rate to the consumer-smoke config was dropped as sprawl.
Areas Affected
Platform/FreeRtos/(tier 2) and theSolidSyslogSysUpTimeFunctiondoc commentin
Core/Interface/(tier 1) — comments only, no signature or behaviour change.Documentation and
misra_suppressions.txt. Integrators outside the supportedtick envelope get a building pack where they previously got a compile error.
Summary by CodeRabbit
Documentation
sysUpTimerequirements for monotonic RFC 3418TimeTicks, including rollover and counter scaling behaviour.Compatibility