Skip to content

Generate parameterUpdated calls in parametersLoaded - #1097

Open
durasi wants to merge 3 commits into
nasa:mainfrom
durasi:feature/5697-parameters-loaded-notify
Open

Generate parameterUpdated calls in parametersLoaded#1097
durasi wants to merge 3 commits into
nasa:mainfrom
durasi:feature/5697-parameters-loaded-notify

Conversation

@durasi

@durasi durasi commented Sep 3, 2026

Copy link
Copy Markdown

Implements #1098.

Per @Willmac16's guidance on the issue, the calls go in the parametersLoaded hook rather than the loadParameters body.

The generated parametersLoaded base implementation was empty, so a component that centralizes update handling in parameterUpdated did not get called for each parameter at load time and had to iterate manually. It now calls parameterUpdated once per parameter, giving full coverage on load with no manual iteration.

This is a behavior change: parameterUpdated now fires for every parameter when parameters are loaded. Because parametersLoaded stays a virtual hook, a component can override it to restore the previous no-op behavior, for example to avoid double-handling when updates are managed elsewhere.

Updated the affected component and tester base reference outputs.

Testing: Scala unit tests pass (633). Component command-line tests pass, reference outputs regenerated.

The generated parametersLoaded base implementation was empty, so a component that centralizes update handling in parameterUpdated did not get called for each parameter at load time and had to iterate manually. It now calls parameterUpdated once per parameter, giving full coverage on load with no manual iteration.

This is a behavior change: parameterUpdated now fires for every parameter when parameters are loaded. Because parametersLoaded stays a virtual hook, a component can override it to restore the previous no-op behavior, for example to avoid double-handling when updates are managed elsewhere.

Updated the affected component and tester base reference outputs.

Signed-off-by: Seçkin Sefa Durası <i@seckin.ws>
@bocchino bocchino changed the title Generate parameterUpdated calls in parametersLoaded (#5697) Generate parameterUpdated calls in parametersLoaded Sep 8, 2026
Wrap each generated parameterUpdated call in a VALID check so
notification only fires for successfully loaded parameters, per review.
Also drop the dead emptyparams branch, since the hook is only
generated when the component has parameters. Regenerated component
reference outputs.
@durasi

durasi commented Sep 8, 2026

Copy link
Copy Markdown
Author

I pushed in 1802ae8.

Each parameterUpdated call is now wrapped in a VALID check, reusing the existing checkValidityFlag helper, so notification only fires for parameters that loaded successfully:

if (this->m_param_ParamU32_valid == Fw::ParamValid::VALID) {
this->parameterUpdated(PARAMID_PARAMU32);
}

I also removed the dead emptyparams branch you flagged, since the hook is only generated when the component has parameters. Reference outputs regenerated; component tests pass.

On the parameterLoaded hook idea: I think that's a good addition separating "loaded" from "updated" avoids the information loss you mentioned. I'd suggest keeping this PR to the validity guarded behavior and opening a separate issue for the new hook, so this one stays focused. Happy to take that on as a followup if you agree.

@bocchino

bocchino commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks!

I'd suggest keeping this PR to the validity guarded behavior and opening a separate issue for the new hook, so this one stays focused.

I discussed this with @LeStarch. We would like to add the parameterLoaded function now, since it is a small change and it is closely related to this PR. Can you go ahead and add that? It should take the parameter ID and the validity status as arguments. By default it should call parameterUpdated with the ID if FW_PARAM_OK is true for the validity status.

@bocchino

bocchino commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Also, let's call the parameterLoaded function as each parameter is loaded, and not all in a block in parametersLoaded. Let's revert parametersLoaded to doing nothing by default.

Add a parameterLoaded(id, valid) hook that by default calls
parameterUpdated when FW_PARAM_OK holds (valid or default). Call it
from the load path as each parameter is loaded, and keep
parametersLoaded as a do-nothing notifier by default. Include
Fw/Prm/ParamValid.hpp for FW_PARAM_OK. Regenerated reference outputs.
@durasi

durasi commented Sep 8, 2026

Copy link
Copy Markdown
Author

I pushed in ed7513c

Replaced the manual VALID check with FW_PARAM_OK(valid), so both VALID and DEFAULT are accepted. Added the Fw/Prm/ParamValid.hpp include to the generated .cpp so the macro is defined.
Added the parameterLoaded(FwPrmIdType id, Fw::ParamValid valid) hook. By default it calls parameterUpdated(id) when FW_PARAM_OK(valid) holds.
It's now called from the load path as each parameter is loaded, rather than in a block. parametersLoaded() is back to doing nothing by default.

Reference outputs regenerated; fpp-to-cpp tests and unit tests pass.

Comment on lines +147 to +153
val prmValidHeader = if hasParameters then List("Fw/Prm/ParamValid.hpp") else Nil
val userHeaders = (List(
"Fw/Types/Assert.hpp",
"Fw/Types/ExternalString.hpp",
"Fw/Types/String.hpp",
s.getIncludePath(componentSymbol, fileName)
).sorted.map(CppWriter.headerString).flatMap({
) ++ prmValidHeader).sorted.map(CppWriter.headerString).flatMap({

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.

Suggested change
val prmValidHeader = if hasParameters then List("Fw/Prm/ParamValid.hpp") else Nil
val userHeaders = (List(
"Fw/Types/Assert.hpp",
"Fw/Types/ExternalString.hpp",
"Fw/Types/String.hpp",
s.getIncludePath(componentSymbol, fileName)
).sorted.map(CppWriter.headerString).flatMap({
) ++ prmValidHeader).sorted.map(CppWriter.headerString).flatMap({
val userHeaders = List.concat(
guardedList (hasParameters) (List("Fw/Prm/ParamValid.hpp")),
List(
"Fw/Types/Assert.hpp",
"Fw/Types/ExternalString.hpp",
"Fw/Types/String.hpp",
s.getIncludePath(componentSymbol, fileName)
)
).sorted.map(CppWriter.headerString).flatMap({

getParamUpdateHookFunction,
getParamLoadHookFunction
getParamLoadHookFunction,
getParamLoadedHookFunction

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.

For consistency, let's rename getParamLoadedHookFunction to getParamsLoadHookFunction.

@bocchino

bocchino commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

@durasi This is looking good. I made a couple more coding suggestions. Can you confirm that the unit tests pass in F Prime when you use this build of FPP? You can use the procedure here: https://github.com/nasa/fpp/wiki/Adding-New-Features-to-FPP#integration-with-f-prime. If this change causes anything to break in F Prime, then we will need to make a tandem PR to F Prime.

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.

2 participants