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.

Sign up for GitHub

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

Jump to bottom

drivers: timer: nrf_rtc_timer: Change type of channel argument #33408

Merged
merged 2 commits into from
Mar 25, 2021
Merged

drivers: timer: nrf_rtc_timer: Change type of channel argument #33408

merged 2 commits into from
Mar 25, 2021

Conversation

nordic-krch
Copy link
Contributor

There was an inconsistency in the API as z_nrf_rtc_timer_chan_alloc returned int but other function were using uint32_t for channel argument. Updated api to use int everywhere.

Signed-off-by: Krzysztof Chruscinski [email protected]

@github-actions github-actions bot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test area: Timer Timer labels Mar 17, 2021
@hubertmis
Copy link
Member

@jciupis, please have a look if it would require any changes in 802.15.4 platform integration.

@jciupis
Copy link

jciupis commented Mar 17, 2021

@hubertmis yes, the 802.15.4 platform integration (namely this file) is going to have to be aligned to use the modified API properly. The changes seem trivial though.

@hubertmis
Copy link
Member

@jciupis, would merging this PR break 802.15.4 builds? Should hal_nordic revision be updated within this PR?

@jciupis
Copy link

jciupis commented Mar 17, 2021

@hubertmis As far as I know, the file that would require changes is currently in zephyr/modules/hal_nordic/nrf_802154 and therefore could be changed in this PR without updating revision of hal_nordic repo. Since the necessary changes would revolve around integer signedness, I don't expect this PR to break the build - built-in integer conversion should handle it. That said, the 802.15.4 platform should be aligned nevertheless.

hubertmis
hubertmis approved these changes Mar 17, 2021
View reviewed changes
Copy link
Member

@hubertmis hubertmis left a comment

Choose a reason for hiding this comment

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

I've built an application for nRF53 net core from this pull request. There were no compiler warnings in 802.15.4 code. Only in nrf_rtc_timer.c that were also spotted by buildkite.

@nordic-krch
Copy link
Contributor Author

Fixing warnings, Updated code in zephyr/modules/hal_nordic/nrf_802154. @jciupis can you take a look?

@jciupis
Copy link

jciupis commented Mar 18, 2021

@nordic-krch LGTM 👍

@nordic-krch
Copy link
Contributor Author

@anangl can you take a look?

@nordic-krch nordic-krch force-pushed the nrf_rtc_timer_fixes branch 2 times, most recently from f5e9471 to a7f431d Compare March 24, 2021 16:13
@nashif
Copy link
Member

nashif commented Mar 25, 2021

this violates rule 4.6:

"typedefs that indicate size and signedness should be used in place of the basic numerical types"

Maybe we should change everything to use int32_t instead?

@nordic-krch
Copy link
Contributor Author

nordic-krch commented Mar 25, 2021 •
edited
Loading

@nashif there are a lot of api's in zephyr which return int with negative being error code and non-negative hold valid information. Are we going to apply this rule in general? I'm not against it, just want to know for future work if i should stick to this rule in my code and during review.

@nashif
Copy link
Member

nashif commented Mar 25, 2021

Are we going to apply this rule in general?

yes, it is a daunting task, but this is the goal. It would be nice if new code already start following this now...

@nordic-krch
drivers: timer: nrf_rtc_timer: Change type of channel argument
c79577f 
There was an inconsistency in the API as z_nrf_rtc_timer_chan_alloc
returned int but other function were using uint32_t for channel
argument. Updated api to use int32_t everywhere.

Update nrf_802154 driver which was using this api to use int32_t.

Signed-off-by: Krzysztof Chruscinski <[email protected]>
@nordic-krch
tests: drivers: timer: nrf_rtc_timer: Minor fixes
5d12b24 
Changed chan type to int in the test_timeout function which follows
change in the api to always use int for channel parameter.

Added assert to check that channel was successfully allocated in
test_resetting_cc().

Signed-off-by: Krzysztof Chruscinski <[email protected]>
@nordic-krch
Copy link
Contributor Author

Updated to use int32_t

@carlescufi carlescufi merged commit 6b026c5 into zephyrproject-rtos:master Mar 25, 2021
This was referenced Apr 7, 2021
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anangl anangl anangl approved these changes

@hubertmis hubertmis hubertmis approved these changes

@carlescufi carlescufi Awaiting requested review from carlescufi

@galak galak Awaiting requested review from galak

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@nashif nashif Awaiting requested review from nashif

@andyross andyross Awaiting requested review from andyross

@KangJianX KangJianX Awaiting requested review from KangJianX

Assignees

@andyross andyross

Labels
area: API Changes to public APIs area: Modules area: Tests Issues related to a particular existing or missing test area: Timer Timer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nordic-krch @hubertmis @jciupis @nashif @anangl @andyross @carlescufi