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

Ticker - kl25z bugfix for handling events in the past #3982

Merged
merged 3 commits into from
Apr 6, 2017
Merged

Ticker - kl25z bugfix for handling events in the past #3982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
094d9ac
Fix crash with events in the past
mjrgh Mar 21, 2017
72ccf0b
Force events into the future
mjrgh Mar 21, 2017
15c740f
Update us_ticker.c
mjrgh Mar 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions hal/mbed_ticker_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,17 @@ void ticker_insert_event(const ticker_data_t *const data, ticker_event_t *obj, t
prev = p;
p = p->next;
}

/* if we're at the end p will be NULL, which is correct */
obj->next = p;
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you checked the git history? I recall this was already moved from this location down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been the same since at least rev 107. If it was moved down, whoever did it didn't think it through clearly enough, because it's clearly an error: it leaves the list in an inconsistent state before making a call that can end up back in a function here that traverses or manipulates the list. This change looks plainly correct to me.

Copy link
Member

Choose a reason for hiding this comment

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

This change is not needed if us_ticker_set_interrupt does nothing more than set the interrupt.
Anyway, it shouldn't hurt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Observe that this is in the generic HAL and us_ticker_set_interrupt is KL25Z specific. So "if us_ticker_set_interrupt does nothing more than set the interrupt" is only true for KL25Z and only after this change. And in any case, it's still not a good idea to leave a global resource in a corrupted state before calling an external function.

Copy link
Member

Choose a reason for hiding this comment

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

it's still not a good idea to leave a global resource in a corrupted state before calling an external function.

100% agreed.

Observe that this is in the generic HAL and us_ticker_set_interrupt is KL25Z specific. So "if us_ticker_set_interrupt does nothing more than set the interrupt" is only true for KL25Z and only after this change

It is partially true: us_ticker_set_interrupt is indeed not supposed to call us_ticker_irq_handler but it is difficult to write compliant function because the description of hal functions doesn't cover/define details such as side effect allowed by implementation or expected behaviours for corner cases.


/* if prev is NULL we're at the head */
if (prev == NULL) {
data->queue->head = obj;
data->interface->set_interrupt(timestamp);
} else {
prev->next = obj;
}
/* if we're at the end p will be NULL, which is correct */
obj->next = p;

core_util_critical_section_exit();
}
Expand Down
10 changes: 5 additions & 5 deletions targets/TARGET_Freescale/TARGET_KLXX/us_ticker.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,11 +187,11 @@ static void lptmr_isr(void) {
void us_ticker_set_interrupt(timestamp_t timestamp) {
int delta = (int)((uint32_t)timestamp - us_ticker_read());
if (delta <= 0) {
// This event was in the past:
us_ticker_irq_handler();
return;
}
// This event was in the past. Force it into the very near
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't be better to set timer peripheral flag (pending IRQ, if it is available?) ? However, calling irq handle should not happen here

Copy link
Member

Choose a reason for hiding this comment

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

It is undecidable to know if an event if in the past or in the future because the counter wrap.
Given that information int delta = (int)((uint32_t)timestamp - us_ticker_read()); can be negative for a lot of valid value (whenever timestamp >= us_ticker_read() + 0x80000000 ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a limitation of the API, and it's not related to the change in question, as that's the test the existing code uses. (But for what it's worth, it's not valid in the API to create events more than 2^31-1 us in the future in the first place - this is mentioned in at least a few of the time-related APIs.)

// future instead.
delta = 1;
}
us_ticker_int_counter = (uint32_t)(delta >> 16);
us_ticker_int_remainder = (uint16_t)(0xFFFF & delta);
if (us_ticker_int_counter > 0) {
Expand Down