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

Another implementation to enabled embedded-hal 1.0.0 based on esp-hal #15

Closed
wants to merge 2 commits into from

Conversation

zephyr-atomi
Copy link

Enabled embedded-hal 1.0.0.

I'm not keen on maintaining compatibility with both embedded-hal 1.0.0 and 0.2.7 in my project, so it might be a good idea to upgrade. Also:

  • Replaced "esp32-hal" to "esp-hal" to accommodate more ESP series chips, such as ESP32c3 (https://github.com/zephyr-atomi/esp32c3-exp).
  • However, this raises another issue: is a specific backend, for example "esp32", a good idea? Typically, non-ESP32 users might skip the provided esp32-interrupt and use their own backend (esp32c3 / stm32f1xx / rp2040 / ...). Since "loadcell" is a HAL-level library, it might be better not to lock it to just ESP32. Maybe we can consider removing the interrupt; after all, implementing interrupts seems challenging in embedded-hal: How to integrate interrupts into embedded-hal rust-embedded/embedded-hal#57

Zephyr Guo added 2 commits May 10, 2024 09:17
I'm not keen on maintaining compatibility with both embedded-hal 1.0.0 and 0.2.7 in my project, so it might be a good idea to upgrade. Also:

- Replaced "esp32-hal" to "esp-hal" to accommodate more ESP series chips, such as ESP32c3 (https://github.com/zephyr-atomi/esp32c3-exp).
- However, this raises another issue: is a specific backend, for example "esp32", a good idea? Typically, non-ESP32 users might skip the provided esp32-interrupt and use their own backend (esp32c3 / stm32f1xx / rp2040 / ...). Since "loadcell" is a HAL-level library, it might be better not to lock it to just ESP32. Maybe we can consider removing the interrupt; after all, implementing interrupts seems challenging in embedded-hal: rust-embedded/embedded-hal#57
Delay: embedded_hal::blocking::delay::DelayUs<u32>,
SckPin: OutputPin + embedded_hal::digital::OutputPin<Error = Infallible>,
DTPin: InputPin + embedded_hal::digital::InputPin<Error = Infallible>,
Delay: embedded_hal::delay::DelayNs,
Copy link
Owner

Choose a reason for hiding this comment

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

If you change this from uS to nS, you will need to update the calls by a factor of 1000

@DaneSlattery
Copy link
Owner

Hi Zephyr

Thank you for the PR. I think it is good to bring the crate up to v1 of the hal now that it has stabilised. I also think the esp-hal has become a lot more stable now.

To your further point, note that the base version of this library is actually compatible with embedded-hal, which means it is not tied to a specific device. esp-hal is an optional dependency.

Interrupts are a mess at the moment, but I think the base we have implemented for now is a good one, and defining the interface via traits makes it pretty easy to extend. Once embedded-hal decides on an appropriate agnostic interrupt definition, we should definitely update the traits in this crate.

The interrupt feature should and can be extended to support more devices via more flags and config checks, but I am not going to do that since I'm fully committed to the ESP32 ;-) .

I, of course, welcome any other implementations for interrupts on other devices.

@zephyr-atomi
Copy link
Author

Hi Dane,

Thanks for your reply!

Yes, I realize that esp32-hal is an optional dependency, primarily activated when using interrupts. If you are focused on ESP32, keeping esp-hal is fine. My system uses RP2040 / STM32 / ESP (mostly ESP32C3), so I encounter the above issue.

Do you have any suggestions for this code? If you are inclined to merge this PR, I can make changes based on your suggestions for you to merge. Or, you could write a commit yourself to enable embedded-hal 1.0.0. My main interest is in using the loadcell crate with latest embedded-hal.

Best Regards,
Zephyr

@DaneSlattery
Copy link
Owner

Hi Zephyr

I am happy to merge your PR! I have made one suggestion that addresses the unit of our delay class.

I would also encourage you to share the code required to get the examples working on your RP2040 or STM32, based on embedded-hal-1.0. That way we can probably add those targets for examples.

@DaneSlattery
Copy link
Owner

Hi Zephyr, any plans to look into the units of the delay class? I think we need a multiplier if we change from ms to us

@DaneSlattery
Copy link
Owner

This has gone stale

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