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

Error if OS tickrate is changed #5264

Merged
merged 1 commit into from Oct 13, 2017

Conversation

Projects
None yet
6 participants
@c1728p9
Contributor

c1728p9 commented Oct 5, 2017

The current mbed-os drivers rely on a tickrate of 1ms for timing. This means that if OS_TICK_FREQ is set to any value other than 1000 then mbed-os driver will no longer delay for the correct amount of time. To prevent this from happening this patch triggers a compile time error if a tickrate other than 1m is used.

Add error if OS tickrate is changed
The current mbed-os drivers rely on a tickrate of 1ms for timing.
This means that if OS_TICK_FREQ is set to any value other than 1000
then mbed-os driver will no longer delay for the correct amount of
time. To prevent this from happening this patch triggers a compile
time error if a tickrate other than 1m is used.
@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 5, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 6, 2017

Is this "locking" required? I recall we had earlier discussion about this during tickless where we calculate the freq in the runtime as there is API to get it.

We always assumed that tick is 1ms, and I recall it is in our codebase used in various primitives. We could potentionaly update the codebase to calculcate it runtime to enable a user to change it, but the question this patch is trying to answer - is it worth considering?

I am inclined to have this assumption - tick 1ms. What others think?

@pan-

This comment has been minimized.

Member

pan- commented Oct 6, 2017

At the current moment the assumption that 1 tick equal 1ms is present in the code. For example cornerstone functions like Thread::wait make that assumption.

Triggering error if OS_TICK_FREQ is not equal to 1000 is a good way to prevent invalid code to be compiled.

I am inclined to have this assumption - tick 1ms. What others think?

I think it is good enough for now however changing the tickrate may help a (very little) bit to save power. That may be a good things to have conversion functions tick_to_ms and ms_to_tick - even if they just return there input - and start to use them in new code or refactored code. If one day, it becomes a requirement to support a tick frequency not equal to 1000Hz then less code will have to be fixed.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 10, 2017

retest uvisor

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 10, 2017

I think it is good enough for now however changing the tickrate may help a (very little) bit to save power. That may be a good things to have conversion functions tick_to_ms and ms_to_tick - even if they just return there input - and start to use them in new code or refactored code. If one day, it becomes a requirement to support a tick frequency not equal to 1000Hz then less code will have to be fixed.

What do you think @c1728p9 ? would this be feasiable to do

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 10, 2017

i agree it's good for now, but in general we should do as @pan- suggested and fix up our OS to make it more robust.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 10, 2017

With tickless I don't know if there is much of a reason to adjust the tickrate. The OS will stay in sleep mode without ticking as long as there is nothing to do. Because of this, the only benefit I can see of adjusting the tickrate is less overhead for context switching if multiple threads are ready to run since context switching will occur less frequently.

The addition of tick_to_ms and ms_to_tick has the advantage of allowing the tickrate to be adjusted. On the flip side, certain tick frequencies will force division to be done on every conversion which may effect performance on devices without hardware division, such as the nrf51. It also makes rollover handling more complicated since uint32_t ticks and ms don't overflow at the same time.

I'm not against having a variable tickrate, but at the moment the to me it seems the downsides outweigh the benefits. @pan- or anyone else, did you have use cases in mind for a variable tickrate?

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 10, 2017

I would say we should leave it as it is, with this change. And address it if it pops out (is requested).

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 10, 2017

Build : SUCCESS

Build number : 73
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5264/

Triggering tests

/test mbed-os

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 12, 2017

retest uvisor

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 12, 2017

Waiting for uvisor CI to be finished.

Once green, 4 ppl 👍 for this, going in

@theotherjimmy theotherjimmy merged commit b9a48a4 into ARMmbed:master Oct 13, 2017

5 checks passed

Cam-CI uvisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment