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

Add I2C, SPI, and ADC functionality, Take 2 #17

Merged
merged 84 commits into from
Jan 20, 2025

Conversation

RossPorter506
Copy link
Contributor

Hi,

This is based on the work from #16, but as the authors haven't responded in over a year I took the time to touch up the PR myself. I fixed the issues mentioned in #16 and also took a look over the rest of the code. I made some changes myself, notably making the examples compile again and adding a new example that uses the newly added delay_ms() functionality.

There were a couple issues with the previous PR, namely the from_u8 macro in eusci.rs, whose implementation of From<u8> for some of the enums was endlessly recursive in a rather subtle way (the From implementation called .into(), which is itself derived from the From implementation). I presume this function was never called in the examples they wrote. I manually reimplemented this. The manual implementation revealed an issue in the Ucastp implementation: There is a reserved register value that isn't exposed in the user-facing enum, so right now it just panics in this case. Let me know if you want to do anything different here.

The serial changes also reintroduced some potential panics (mostly divisions) which caused the examples that use panic_never to fail to compile, so I rewrote some of that part. There's lots of unchecked unwraps in there, so I would appreciate another pair of eyes on this part.

I also took the opportunity to fix some mostly cosmetic issues my linter picked up on in the rest of the codebase, such as implementing Into instead of From, useless .into() calls, things like that.

Let me know if you'd like any changes.

@RossPorter506
Copy link
Contributor Author

@RossPorter506 Is this PR complete or are you planning to add more examples?

I'm planning to add more examples and do some final tests on a dev board. Hopefully done by the end of this week or so.

@RossPorter506
Copy link
Contributor Author

Sending SPI packets was tested against a logic analyzer, which showed the packets were well-formed, in the correct mode, etc. I also did some rudimentary tests for recieving SPI packets by just pulling the bus high or low and seeing a packet of all 1's or 0's respectively.
One thing I ran into was that the SPI code forced 4-pin mode, which uses STE as the only chip select. This doesn't work for multi-slave systems, so I added an option to instead use 3-pin mode, where the user is expected to assert and de-assert the chip select pins manually. It's a little bit of a footgun, as it's possible the user may forget to twiddle the chip select pin in this mode. It would seem reasonable to have separate read() and write() methods for this mode that additionally take in a generic GPIO pin as the chip select, which we can twiddle as needed. This wouldn't fit with the current embedded_hal traits though.

My I2C testing was a bit more limited as I would need an actual device to ACK to continue any conversation, but the logic analyzer confirmed the initial setup packet (e.g. address) was correct.

I plan to write the ADC example and test it soon.

src/spi.rs Show resolved Hide resolved
src/i2c.rs Show resolved Hide resolved
examples/i2c.rs Outdated Show resolved Hide resolved
examples/i2c.rs Outdated Show resolved Hide resolved
examples/spi.rs Outdated Show resolved Hide resolved
src/spi.rs Show resolved Hide resolved
@YuhanLiin
Copy link
Owner

Sending SPI packets was tested against a logic analyzer, which showed the packets were well-formed, in the correct mode, etc. I also did some rudimentary tests for recieving SPI packets by just pulling the bus high or low and seeing a packet of all 1's or 0's respectively. One thing I ran into was that the SPI code forced 4-pin mode, which uses STE as the only chip select. This doesn't work for multi-slave systems, so I added an option to instead use 3-pin mode, where the user is expected to assert and de-assert the chip select pins manually. It's a little bit of a footgun, as it's possible the user may forget to twiddle the chip select pin in this mode. It would seem reasonable to have separate read() and write() methods for this mode that additionally take in a generic GPIO pin as the chip select, which we can twiddle as needed. This wouldn't fit with the current embedded_hal traits though.

My I2C testing was a bit more limited as I would need an actual device to ACK to continue any conversation, but the logic analyzer confirmed the initial setup packet (e.g. address) was correct.

I plan to write the ADC example and test it soon.

Correct me if I'm wrong, but in 3-wire SPI the "chip control" pins have no correlation whatsoever with the SPI controller right? There may even be multiple? In that case I don't think we can "fool-proof" the 3-wire SPI read/write API. Even if we take a generic GPIO pin, we can't actually constrain it to the "correct" chip select pin, and we don't even know if it's active high or active low.

@RossPorter506
Copy link
Contributor Author

Correct me if I'm wrong, but in 3-wire SPI the "chip control" pins have no correlation whatsoever with the SPI controller right? There may even be multiple? In that case I don't think we can "fool-proof" the 3-wire SPI read/write API. Even if we take a generic GPIO pin, we can't actually constrain it to the "correct" chip select pin, and we don't even know if it's active high or active low.

Yeah, there could be multiple CS pins and they could be any arbitrary pins. I agree that attempting a completely fool-proof implementation is a losing battle, but we shouldn't let perfect get in the way of good. I don't think we should try and constrain the CS pin because as you say anything could be a CS pin, though requiring a pin be passed as a parameter seems like a reasonable middle ground to me, it's up to the user to make sure that they pass in the CS pin that actually corresponds to the device they want to talk to. There's no way we can check this for them.

If we make the assumption that the CS pin is in the idle state initially (as opposed to assuming the user has dealt with the CS pin correctly, which is arguably a bigger ask), we could take a ToggleableOutputPin and just toggle the CS pin before and after transmission, that way we don't need to know the polarity.

@RossPorter506
Copy link
Contributor Author

If we make the assumption that the CS pin is in the idle state initially [...]

The more I think about this the less I like it. I will keep it as-is for now.

In a multi-slave system some slaves may require a different SPI mode.
The previous generic implementation of OneShot would force you to always define the type of the output to disambiguate. Since `adc_get_result()` always returns a u16 just implement OneShot for u16.
src/spi.rs Show resolved Hide resolved
examples/adc.rs Outdated Show resolved Hide resolved
examples/adc.rs Outdated Show resolved Hide resolved
src/adc.rs Show resolved Hide resolved
@RossPorter506
Copy link
Contributor Author

RossPorter506 commented Jan 20, 2025

I just noticed the ADC module only exposes the ADC channels attached to external pins. There's a few extra channels that you can also measure from (Table 6-21, user guide). I'll quickly add these.
Edit: See fc0c486

@YuhanLiin YuhanLiin merged commit d3c45f8 into YuhanLiin:master Jan 20, 2025
1 check passed
@YuhanLiin
Copy link
Owner

Thank you for the PR!

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.

4 participants