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

Implement initial state for output pins #168

Merged
merged 1 commit into from
Mar 23, 2021

Conversation

Javier-varez
Copy link
Contributor

Currently there is no way to set the initial state of an output pin, which can be very problematic for critical signals, as by default they could be in either high or low depending on the corresponding bit of the port register.

This change provides an alternative API for users who want to set the initial state of a pin when switching to output.

@Rahix
Copy link
Owner

Rahix commented Mar 22, 2021

Hi, thanks a lot for the PR! This is a very sensible addition, I agree.

However, we're currently reworking the design of avr-hal, so the version you added this to is not going to exist for much longer... You can check out the new crates on the next branch. This is the relevant part:

/// Convert this pin into an output pin. See [Digital Output](#digital-output).
pub fn into_output(mut self) -> Pin<mode::Output, PIN> {
unsafe { self.pin.make_output() };
Pin {
pin: self.pin,
_mode: PhantomData,
}
}

There was a previous comment by someone else about this also leaving the pin in a non-deterministic state depending on what was configured beforehand. I think the best course of action is to make into_output() always put the pin into a low state and adding a second method into_output_high() which always puts it high. E.g.

    pub fn into_output(mut self) -> Pin<mode::Output, PIN> {
        unsafe {
			self.pin.out_clear();
			self.pin.make_output();
		}
        Pin {
            pin: self.pin,
            _mode: PhantomData,
        }
    }

What do you think? Would you be interested on reimplementing this based on the new code in the next branch?

@Rahix Rahix added hal-api API design for the different components of avr-hal hal-generic Related to MCU generic parts of avr-hal labels Mar 22, 2021
@stappersg
Copy link
Contributor

stappersg commented Mar 22, 2021 via email

@Javier-varez
Copy link
Contributor Author

Hi! Thanks for the quick reply! I agree that it would be a good idea as well to configure the port as low when into_output is called. I will move this PR to the next branch!

@Javier-varez Javier-varez changed the base branch from master to next March 22, 2021 22:22
Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

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

Neat, thanks a lot! :)

I guess at some point we might want to document why one would want to use this method, maybe with an example, but I'll be doing a sweep over the entire codebase and add all such things once #130 is wrapped up.

@Rahix Rahix merged commit 4b7262c into Rahix:next Mar 23, 2021
@Javier-varez Javier-varez deleted the initial_state branch March 23, 2021 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hal-api API design for the different components of avr-hal hal-generic Related to MCU generic parts of avr-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants