Skip to content

atmel-samd: Fix a tick hang when we hit the exact timing we're going for#771

Closed
tannewt wants to merge 1 commit into
adafruit:masterfrom
tannewt:tick_fix
Closed

atmel-samd: Fix a tick hang when we hit the exact timing we're going for#771
tannewt wants to merge 1 commit into
adafruit:masterfrom
tannewt:tick_fix

Conversation

@tannewt
Copy link
Copy Markdown
Member

@tannewt tannewt commented Apr 19, 2018

The SysTick interrupt happens when the value is 0 so its never
readable when 0 and interrupts are enabled.

…for.

The SysTick interrupt happens when the value is 0 so its never
readable when 0 and interrupts are enabled.
@tannewt tannewt added this to the 3.0 Alpha milestone Apr 19, 2018
@tannewt tannewt requested a review from dhalbert April 19, 2018 18:43
@jerryneedell
Copy link
Copy Markdown
Collaborator

@tannewt Won't this ignore any delay < 1000 if it happens to fit in before the next tick. say us = 50 and us_between ticks is >50 then this will just return.

@jerryneedell
Copy link
Copy Markdown
Collaborator

sorry - it will just ignore a delay that happens to meet the criterion us=us_between_ticks -- may happen, may not.

@jerryneedell
Copy link
Copy Markdown
Collaborator

If this is not urgent, I will combine it with #765 and submit it tonight.

@dhalbert
Copy link
Copy Markdown
Collaborator

(@tannewt and I are sitting next to each other and discussing this.) us_between_ticks means us_to_next_tick. So if us < 1000 it will skip the while loop. And if us_between_ticks is < us, then the subtraction on line 69 is negative, but it's unsigned ints, so it will be a very large number. Either way, it's unintended. We think we just need to rewrite this routine completely.

@jerryneedell
Copy link
Copy Markdown
Collaborator

I tried to deal with that in #765. I think it can handle this with a minor change. Or we can just close it and start over

@jerryneedell
Copy link
Copy Markdown
Collaborator

jerryneedell commented Apr 19, 2018

More time to write now.. In #765 I think I took care of the issue with the large negative numbers and decoupled tick_delay from ticks_ms so it does not care if interrupts are enabled or not. I you think this is a good approach, I think I can deal with the new case raised here, but if you prefer to just scrap it and start over, that is fine with me.

void tick_delay(uint32_t us) {
    uint32_t ticks_per_us = common_hal_mcu_processor_get_frequency() / 1000 / 1000;
    uint32_t us_between_ticks = SysTick->VAL / ticks_per_us;
    uint32_t start_tick;
    while (us > 1000) {
        start_tick=SysTick->VAL;  // wait for SysTick->VAL to  RESET
        while (SysTick->VAL < start_tick) {}
        us -= us_between_ticks;
        us_between_ticks = 1000;
    }
    if(us&&(us < us_between_ticks)){
        while (SysTick->VAL > ((us_between_ticks - us) * ticks_per_us)) {}
    }
    else {
        start_tick=SysTick->VAL;  // wait for SysTick->VAL to  RESET
        while (SysTick->VAL < start_tick) {}
        us -= us_between_ticks;
        if(us){
            while (SysTick->VAL > ((1000 - us) * ticks_per_us)) {}
        }
    }
}

@dhalbert
Copy link
Copy Markdown
Collaborator

@jerryneedell I hadn't read the #765 code. Yes, please try this fix as well, if you have a good way to test. Thanks!

@dhalbert
Copy link
Copy Markdown
Collaborator

@jerryneedell Can you rename us_between_ticks to us_to_next_tick or some similar name?

@jerryneedell
Copy link
Copy Markdown
Collaborator

NP - I'll try to come up with a better version tonight or tomorrow. I can add the check to see if interrupt are enabled that we also talked about in that PR.

@jerryneedell
Copy link
Copy Markdown
Collaborator

I think the DHT is a good test since it uses pulseio and tick_delay. I have been using that for the testing so far.

@tannewt
Copy link
Copy Markdown
Member Author

tannewt commented Apr 20, 2018

Closing in favor of #765

@tannewt tannewt closed this Apr 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants