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

mips32r2_common: timer fixes #8817

Merged
merged 2 commits into from
Apr 13, 2018
Merged

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Mar 22, 2018

Contribution description

Fix return value for timer_ function that should return 1 on success.

mips32r2_common already implements timer_set so it should not be provided by
periph_common/timer to avoid multiple definition errors hidden by the linker.

The firmware was using the one from mips32r2_common before (binary checked).
So the final linked version is the same.

This PR is using a commit from #8711 with added info in the commit message.

Verification

I checked what was built with:

mips-mti-elf-objdump -d bin/mips-malta/mips32r2_common_periph/timer.o bin/mips-malta/periph_common/timer.o bin/
mips-malta/*.elf | grep -A 30 '<timer_set>:'

And the timer_set implementation is the same before and after the fix.

Issues/PRs references

It is required for #8711 and replaces #8720 which raised other problems.

cladmi and others added 2 commits March 22, 2018 14:21
Fix 'timer_set', 'timer_set_absolute' and 'timer_clear' return value to 1 on
success as documented in the API.
mips32r2_common already implements timer_set so it should not be provided by
periph_common/timer to avoid multiple definition errors currently hidden by the
linker.

The firmware was using the one from `mips32r2_common` before (binary checked).
So behaviour is identical.
@cladmi cladmi added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: MIPS Platform: This PR/issue effects MIPS-based platforms labels Mar 22, 2018
@cladmi cladmi requested review from jnohlgard and neiljay March 22, 2018 13:24
Copy link
Contributor

@neiljay neiljay left a comment

Choose a reason for hiding this comment

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

It looks good to me, I have not checked it on target hardware yet, that may take a few days.

But I cannot see why there would be any issues and you have checked the resultant binary is unchanged.

So if you want to merge this soon to resolve another issue please go ahead.

Is there a plan to fix the race in the generic implementation ?

@cladmi
Copy link
Contributor Author

cladmi commented Apr 9, 2018

@neiljay I have not followed the generic implementation case for the moment, I did not exactly got the problem, so hard to do a PR if I miss some points.

@cladmi
Copy link
Contributor Author

cladmi commented Apr 12, 2018

@neiljay do you think you will have time to test it for the release ?

@neiljay
Copy link
Contributor

neiljay commented Apr 13, 2018

@cladmi Just tested on pic32-wfire with timer_periodic_wakeup and it looks good:

main(): This is RIOT! (Version: 2017.01-devel-1551-gbbd6a-ldt-n-jones-heads/cladmi/pr/ld/mips32r2_timer_set)
slept until 1002496
slept until 2002944
slept until 3002368
slept until 4002816
slept until 5002240
slept until 6002688
slept until 7002112
slept until 8002560
slept until 9003008
slept until 10002432
slept until 11002880
slept until 12002304
slept until 13002752
slept until 14002176
slept until 15002624
slept until 16002048
slept until 17002496
slept until 18002944
slept until 19002368
slept until 20002816
slept until 21002240
slept until 22002688
slept until 23002112
slept until 24002560
slept until 25003008
slept until 26002432
slept until 27002880

@cladmi
Copy link
Contributor Author

cladmi commented Apr 13, 2018

Thanks

@cladmi cladmi merged commit e876bbd into RIOT-OS:master Apr 13, 2018
@cladmi cladmi deleted the pr/ld/mips32r2_timer_set branch April 16, 2018 12:42
maxvankessel pushed a commit to maxvankessel/RIOT that referenced this pull request Apr 20, 2018
@cladmi cladmi added this to the Release 2018.04 milestone Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: MIPS Platform: This PR/issue effects MIPS-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants