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 to embedded-hal v1.0.0 #55

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

Sycrosity
Copy link

@Sycrosity Sycrosity commented Jan 13, 2024

  • Updates Cargo.toml to eh v1.0.0
  • Updated embedded-hal-mock to v0.10.0
  • Replace existing trait impls with eh v1.0.0 ones

This is by no means without it's difficulties - the AdcProxy struct must be dropped, as eh v1.0.0 does not contain traits for Adc, and the tests have not yet been updated - I don't have the time at this moment to implement those.

pub fn acquire_adc<'a>(&'a self) -> crate::AdcProxy<'a, M> {
crate::AdcProxy { mutex: &self.mutex }
}
// pub fn acquire_adc<'a>(&'a self) -> crate::AdcProxy<'a, M> {

Choose a reason for hiding this comment

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

Could you help me understand why this section of the code is commented out, especially since the corresponding test was also removed? I’m curious whether this change addresses a particular issue or if it’s related to clean code practices.

Copy link
Author

@Sycrosity Sycrosity Oct 2, 2024

Choose a reason for hiding this comment

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

As I wrote this code a while ago, I don't fully remember - however I believe it is because there is no implementation of ADC or analogue pins in embedded-hal v1.0.0, whereas there was before. This code can be removed rather than commented out, I just commented it as this may be introduced in a later embedded-hal version (e.g. v1.1.0), for convinence. However deleting this may make more sense.

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