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

Update pal timers #86

Closed
wants to merge 2 commits into from
Closed

Update pal timers #86

wants to merge 2 commits into from

Conversation

eobalski
Copy link

[x] I confirm this contribution is my own and I agree to license it with Apache 2.0.
[x] I confirm the moderators may change the PR before merging it in.
[x] I understand the release model prohibits detailed Git history and my contribution will be recorded to the list at the bottom of CONTRIBUTING.md.

Summary of changes

Updated pal timer implementation for zephyr project, after new k_work API was introduced.

Update variable name to better reflect its meaning.

Signed-off-by: Emil Obalski <emil.obalski@nordicsemi.no>
@eobalski
Copy link
Author

@pdunaj please, have a look.

@eobalski eobalski changed the title Fix/pal timers Update pal timers Jun 23, 2021
timer_data->timeout = (uptime + millisec);
if (timer_data->period) {
timer_data->period = millisec;
}

if (!k_work_cancel_delayable(&timer_data->work)) {
ret = k_work_cancel_delayable(&timer_data->dwork);
Copy link

Choose a reason for hiding this comment

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

can we cancel delayable sync instead?
I don't think PAL timer start or stop would be executed from atomic context

Copy link

Choose a reason for hiding this comment

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

Hi @marcuschangarm , is it the right assumption?

Copy link
Author

Choose a reason for hiding this comment

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

Having it sync is a trip to deadlock with current implementation. We are locking access to timer data. If I make this sync then the work_fn must be finished so that function could return. This will never happen because handler would not be able to lock the mutex (at the beggining of work_fn)

Choose a reason for hiding this comment

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

I don't see anywhere in the code where start/stop is called from interrupt context. It might be called from the work queue itself though.

Copy link

Choose a reason for hiding this comment

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

Thanks.

It might be called from the work queue itself though

I think that we can handle this case.

@eobalski
Copy link
Author

Added a commit using delayable sync when the timer is stopped form context other to system work Q. For now, Im leaving this for reflection.

@teetak01
Copy link
Contributor

Thanks for the contribution @emob-nordic

Do we still expect more changes for this PR?

} else {
/* Stop called from other other thread */
k_mutex_unlock(&timer_data->lock);
k_work_cancel_delayable_sync(&timer_data->dwork, &timer_data->work_sync);
Copy link

Choose a reason for hiding this comment

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

I suggest switching to normal cancel and moving under lock. Then perform cancel sync only on timer delete.
As discussed offline I think that having the cancel outside lock will lead to incoherency between work state and timer data.

I am wondering if the easiest solution would not be to simply throw away the cancel altogether. Callback will handle itself and not call upper layer if timer was stopped. In such case we would only need to call cancel sync on timer delete.

Alternatively we can introduce api lock on start and stop. Or we could rethink if there is a way to have lock removed from callback. Both of these I find a bit complicated. So what would you say about abandoning the cancel in stop?

Copy link
Author

Choose a reason for hiding this comment

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

For the first suggestion I will give it a try.

I don't think it would be the easiest solution to simply let callback handle each stop.
In most cases cancel would remove ongoing work so having this in TimerStop is, imho, expected. If the work could not be removed then we rely on callback finishing the job.

I think cancel should stay in stop. I will put async cancel under lock (in stop) to prevent potential race and add sync in timer delete. Will test a bit and ping back if there is something to be worried about.

@pdunaj
Copy link

pdunaj commented Jun 29, 2021

Do we still expect more changes for this PR?

@teetak01 Let's wait just a bit :)

@eobalski
Copy link
Author

@pdunaj please have a look. I've tested with ncs application and performed tests developed in the past.

Copy link

@pdunaj pdunaj left a comment

Choose a reason for hiding this comment

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

Changing type needs explanation.
Besides that code looks ok.


k_mutex_lock(&timer_data->lock, K_FOREVER);

uint64_t timeout = timer_data->timeout;
int64_t timeout = timer_data->timeout;
Copy link

Choose a reason for hiding this comment

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

why do you use int64_t for timeout?

Copy link

Choose a reason for hiding this comment

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

I don't think this can be negative and there can be an overflow issue

Copy link
Author

Choose a reason for hiding this comment

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

Nothing besides i wanted the type to match to what is returned by k_uptime_get(). I'll revert this change.

uint64_t timeout;
uint64_t period;
int64_t timeout;
int32_t period;
Copy link

Choose a reason for hiding this comment

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

why using int32_t for period?


k_mutex_lock(&timer_data->lock, K_FOREVER);

timer_data->timeout = (uptime + millisec);
timer_data->timeout = (uptime + (int64_t)millisec);
Copy link

Choose a reason for hiding this comment

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

adding signed integers is very dangerous in C as overflow here is an undefined behavior

Update implementation of Timer API:
- Use absolute k_timeout instead of calculated ones when scheduling works.
  One exception is when timer work is handled too early. This should not
  happen but could be handled.
- Cancel sync when timer is deleted.
  In other cases let the timer callback finish the job.
- Remove counting semaphore for timer usages. New API allows to better
  track work state and its not required now.

Signed-off-by: Emil Obalski <emil.obalski@nordicsemi.no>
Copy link

@pdunaj pdunaj left a comment

Choose a reason for hiding this comment

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

Looks ok now.
Personally I would not change the name of the member from work to dwork (to avoid noise).

@marcuschangarm
Copy link

@teetak01 internal PR updated with latest changes.

@teetak01 teetak01 added the Approved PR has been approved and will be integrated to internal development repository label Jul 2, 2021
@teetak01
Copy link
Contributor

teetak01 commented Jul 8, 2021

Released as part of 4.10.0.

@teetak01 teetak01 closed this Jul 8, 2021
@teetak01 teetak01 added this to the 4.10.0 milestone Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved PR has been approved and will be integrated to internal development repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants