-
Notifications
You must be signed in to change notification settings - Fork 139
nrfs: Added explicit enum values #277
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
nrfs: Added explicit enum values #277
Conversation
f512db3
to
d9f5f74
Compare
There still is a lot missing in the other ones
I think assigning fixed enums everywhere will greatly reduce the chance of accidental breakage, when enums are hardcoded people tend to make sure to not insert in the middle or reorder, when they are not it seems the order doesn't matter and one needs to be very careful during reviews to catch enums changing sequence that need to have compatibility kept in mind |
Added explicit enum values for better resistance to change Signed-off-by: Rafal Dyla <[email protected]>
d9f5f74
to
b6aab21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please double check the nrfs_temp.h change so that it didn't reorder, otherwise looks good
#endif | ||
NRFS_TEMP_EVT_REJECT, /** Request rejected. */ | ||
NRFS_TEMP_EVT_REJECT = 2, /** Request rejected. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be reordered? NRF_TEMP_EVT_REJECT assign 1 and NRFS_TEMP_EVT_CHANGE to 2 to make things not change?
I tried searching for NRFS_TEMP_SERVICE_SUBSCRIPTION_ENABLED
and haven't found it, so I assume NRFS_TEMP_EVT_REJECT
used to map to 1 and NRFS_TEMP_EVT_CHANGE wasn't used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried searching for NRFS_TEMP_SERVICE_SUBSCRIPTION_ENABLED and haven't found it, so I assume NRFS_TEMP_EVT_REJECT used to map to 1 and NRFS_TEMP_EVT_CHANGE wasn't used?
These changes are merged in the upstream zephyr and still waiting for merging in the ncs (nrfconnect/sdk-zephyr#3050)
Current order is ok, it ensures compatibility with platforms which still have temp_subscription (nrf9230)
Explicit enum values should now fix the potential problem of using incompatible ncs/soc bundle versions. REJECT will be always interpreted as 2, regardless of whether SCFW has temp_subscription.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicit enum values should now fix the potential problem of using incompatible ncs/soc bundle versions
Nice, thanks!
Added explicit enum values for better resistance to change