Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added paramNotifyChanged() to let apps notify the core when they modify the value of a parameter programmatically #1193

Merged
merged 2 commits into from
Jan 24, 2023

Conversation

ntamas
Copy link
Contributor

@ntamas ntamas commented Jan 21, 2023

This function intends to cater for the use-case when an app modifies the value of a parameter using paramSetInt(), paramSetFloat() or some similar mechanism (i.e. not externally via a CRTP packet but internally). Currently these functions do not call the callbacks associated to the parameter if there is one; paramNotifyChanged() allows the app code to trigger the callback programmatically.

Another option would be to change paramSetInt(), paramSetFloat() and similar functions to call the callback, but I don't know what other implications this change may have so I went for the surgical option instead.

…fy the value of a parameter programmatically
Copy link
Member

@ataffanel ataffanel left a comment

Choose a reason for hiding this comment

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

Good finding, I think the fact that the callback was not called by the internal API function is a bug.

I would say that calling the paramNotifyChanged function from paramSetInt and paramSetFloat is the right things to do: the internal param API should work as much as possible like the external API to avoid bug and surprises.

I am not sure the notify function should be pubic in that case though.

@krichardsson
Copy link
Contributor

I agree with @ataffanel
Sounds reasonable that the callback is called also when a parameter is set from an app in the FW

@ntamas
Copy link
Contributor Author

ntamas commented Jan 23, 2023

Okay, I'll change the PR then, give me a sec :)

@ntamas
Copy link
Contributor Author

ntamas commented Jan 23, 2023

First iteration. Maybe the call to paramNotifyChanged() could actually also be pushed down to paramSet(), but then the callbacks would also be called in persistentParamFromStorage() - do we want that or not?

@krichardsson
Copy link
Contributor

Oh, that is a good question! And a bit scary because it touches on the infinitely complex topic of initialization. The initialization process/sequence is slightly undefined at the moment and in my opinion we really should shape it up to clearly define the expected behavior. For how I would say that we do not call the callback when setting parameters from persisted values.

At the moment there is no case where we have a persistent parameter with a callback so we can decide on what ever solution we want I guess.

@ataffanel
Copy link
Member

I agree, one idea of stored parameter was to 'simulate' the behavior of default value, in that sense not calling the callback for stored parameter sounds like the right behavior.

@krichardsson krichardsson merged commit 9e7e697 into bitcraze:master Jan 24, 2023
@krichardsson
Copy link
Contributor

Thanks!

@ntamas ntamas deleted the feat/param-notify-changed branch January 24, 2023 08:37
@ntamas
Copy link
Contributor Author

ntamas commented Jan 24, 2023

Note that the title of the PR became misleading now that paramNotifyChanged() is not a public function any more. The upside is that apps don't need to do anything explicitly now.

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.

3 participants