Skip to content

fix OneWire timing and DigitalInOut.switch_to_input() #973

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

Merged
merged 5 commits into from
Jun 29, 2018

Conversation

dhalbert
Copy link
Collaborator

OneWire didn't work at all, just sending one long pulse. common_hal_digitalio_digitalinout_switch_to_input() was buggy, actually failing to change the port direction. Fixed, and cleaned up DigitalInOut.c a bit, using hri_ calls to save a bit of time in certain cases, and avoiding some redundant operations.

After that fix, OneWire was sort of working, but acting erratically, sometimes getting a CRC error. This problem was not present in 2.x. OneWire does bit-banging with precise time intervals using non-interruptable time-delay calls for small numbers of microseconds. It turns off interrupts while making these calls. But it was using common_hal_mcu_delay_us(), which calls tick_delay(), which depends on an interrupt to handle SysTick reaching zero. So the time-delay calls were inaccurate in cases when SysTick reached zero would have interrupted, but could not. This was not a problem in 2.x because 2.x did not use SysTick for general time-keeping, but only for delay operations.

Replaced the common_hal_mcu_delay_us() calls in OneWire.c with calls to a new routine just loops to use up time. Loop was calibrated on both SAMD21 and SAMD51 with the standard 48MHz and 120MHz clock frequencies.

Tested on Metro M0 and M4. Fixes #964.

@dhalbert dhalbert requested a review from tannewt June 29, 2018 03:35
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

One simplification and then it should be good to go.

#define DELAY_LOOP_ITERATIONS_PER_US ( (30U*120000000U) / common_hal_mcu_processor_get_frequency())
#endif

void mp_hal_delay_us_loop(uint32_t us) {
Copy link
Member

Choose a reason for hiding this comment

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

No need to add another API. Just make mp_hal_delay_us this implementation. It shouldn't be used for >1ms timing.

@dhalbert
Copy link
Collaborator Author

updated and travis is happy

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Yay! Looks good. Thank you!

@tannewt tannewt merged commit f1248dc into adafruit:3.x Jun 29, 2018
@dhalbert dhalbert deleted the 3.x_onewire_fix branch May 28, 2019 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants