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

Ticker class description update - small interval warning #5045

Merged
merged 1 commit into from Sep 28, 2017

Conversation

Projects
None yet
8 participants
@maciejbocianski
Member

maciejbocianski commented Sep 7, 2017

Description

doxygen update

Status

READY

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 7, 2017

drivers/Ticker.h Outdated
@@ -100,6 +100,10 @@ class Ticker : public TimerEvent, private NonCopyable<Ticker> {
*
* @param func pointer to the function to be called
* @param t the time between calls in micro-seconds
*
* @note setting t to very small value much less than RTOS tick frequency

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

I would reference the t @a t also mention some numbers noting it's platform dependant.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_fix branch Sep 7, 2017

@geky

geky approved these changes Sep 7, 2017

Great addition 👍

Lets just hope we don't end up with a list of 100 platforms with their minimum wait period.

drivers/Ticker.h Outdated
* @note setting @a t to very small value much less than RTOS tick frequency
* can cause system malfunction or even system hang. By default OS tick frequency
* is 1000Hz (1 tick == 1ms) so we schouldn't set much less then 1000us.
* The lowest value which can be set without program hang is H/W dependent

This comment has been minimized.

@c1728p9

c1728p9 Sep 8, 2017

Contributor

This note makes it seem like there is a bug with the some implementations of the ticker which should not be the case. Depending on how much processing is done in the ticker interrupt and current cpu speed the values of t which make the program hang could be arbitrarily large. For example, if your interrupt routine takes 11ms to complete, then a t of 10,000 will cause a system hang.

Maybe a generic warning like this would be more appropriate?

setting @a t to a value shorter that it takes to process the ticker callback will cause the system to hang.

This comment has been minimized.

@bulislaw

bulislaw Sep 11, 2017

Member

Yeah, maybe you are right. Cutting a bit on details and maybe quoting the hardware specific danger zone below 500us(?) which should be safe for most platforms.

This comment has been minimized.

@maciejbocianski

maciejbocianski Sep 11, 2017

Member

I would say that:
timout > T1(calback exec time) + T2(task switch time)

This comment has been minimized.

@maciejbocianski

maciejbocianski Sep 13, 2017

Member

If we set Ticker callback time small enought to fall into endless processing loop there ist still chance to recover from this "hang" state putting some ticker control code inside callback which call detach in suitable moment.

So maybe it's good idea to make comment/warrning short and let programmer to decide what time is good for him.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:ticker_fix branch to 19ee9c1 Sep 18, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 21, 2017

Bump for rereview, the comments were addressed

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 27, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 28, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1404

Test failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 28, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 28, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1412

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 28, 2017

@theotherjimmy theotherjimmy merged commit f57f0d4 into ARMmbed:master Sep 28, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has 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