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 I2C and SPI peripherals for all chips #5

Closed
4 tasks
jessebraham opened this issue Jan 10, 2022 · 8 comments
Closed
4 tasks

Implement I2C and SPI peripherals for all chips #5

jessebraham opened this issue Jan 10, 2022 · 8 comments
Labels
help wanted Extra attention is needed

Comments

@jessebraham
Copy link
Member

jessebraham commented Jan 10, 2022

This is a tracking issue for the remaining communication protocols. With these implemented we should be able to interact with a vast array of sensors and external peripherals.

  • ESP32
    • I2C, SPI
  • ESP32-C3
    • I2C, SPI
  • ESP32-S2
    • I2C, SPI
  • ESP32-S3
    • I2C, SPI
@jessebraham jessebraham added the help wanted Extra attention is needed label Jan 10, 2022
@ducktec
Copy link
Contributor

ducktec commented Feb 20, 2022

I started experimenting with a shared I2C implementation: https://github.com/ducktec/esp-hal/tree/feature/i2c.

For the ESP32-C3, the implemented example (driving a SSD1306 I2C display) works already. For the ESP32 the example at least builds, but does not work yet. I suspect that is because I'm relying on the I2C DATA register in the SVD that does not really exist (espressif/svd#19). S2 and S3 probably don't even build (but are already considered in the shared implementation).

An aspect that I found a bit annoying was that the peripheral's InputSignal and OutputSignal definitions are not part of the PACs, but only available in the individual HAL crates and thus cannot be used with #[cfg] attributes in esp-hal-common. In order to avoid duplicate code in the chip-specific crates, so far I tried working around this with macros. These are then used in the chip-specific HAL crates, but that is taking it's toll on simplicity and readability (and does not yet work for > 1 I2C instance). So, not happy with that solution yet.

I will continue with the implementation, but I'm a bit time-restricted to work on this, thus it might take a while. So, if anybody wants to fast-track this, maybe some pieces of my work can help. 🙂

@jessebraham
Copy link
Member Author

Glad to hear you're interested in the project, thanks for working on this!

I had previously taken a stab at the I2C driver (based heavily on yours from esp32c3-hal) however did not get it to the point of working, so I'm happy to hear you've made it at least a bit further. I also ran into the InputSignal/OutputSignal issue during my attempt; not entirely sure how to address this, but we need to do something. As we implement additional peripheral drivers this problem is only going to compound.

There is no rush at all, so please take your time!

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 24, 2022

@ducktec Thanks for your efforts! I can continue to work on it based on your work - I already cherry-picked your commit and can continue to work on it - I'm sure there will be enough to do once you have more time again or you can pick up my changes if I make progress but don't finish it until you get to it again

@ducktec
Copy link
Contributor

ducktec commented Feb 24, 2022

Alright, then I'll look into another peripheral once I have some more time (mostly weekends). Just let me know if I can be of help with the I2C peripheral.

Do you already have a plan for the InputSignal/OutputSignal issue which I guess will affect many peripherals?

I already looked into ways to get this information into the SVDs and thus PACs:

  • For the OutputSignal this would be pretty easy: the values could be added as enumeratedValue to the OUT_SEL field in the FUNC%s_OUT_SEL_CFG register. Providing them there would yield in a nice enum gpio::func_out_sel_cfg::OUT_SEL_A with the mapping.
  • For the InputSignal the situation is more complicated as the signals map to registers instead of field values. There is an element in the SVD specification > 1.3.1 that seems like a perfect fit: dimArrayIndex. But issue here, this element seems to be completely ignored by svd2rust so far. I can look into the necessary changes for svd2rust if there's no better idea though.

@jessebraham
Copy link
Member Author

I'm generally the one responsible for SVD-related things, and I think I should be able to add these enumerated values. If @bjoernQ doesn't have a different idea then I can take care of the required SVD updates, although as you stated we would likely need to update svd2rust.

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 25, 2022

Interesting idea about having InputSignal / OutputSignal in the SVD - haven't thought about that. Mid-term / long-term it would be definitely good to have that. I think we also need to be able to handle those things in svd-tools (especially svd-patch) - haven't seen that supported there but I might be wrong.

Do you already have a plan for the InputSignal/OutputSignal issue which I guess will affect many peripherals?

For now, my simple solution for that is to move them into the common-hal and include them conditionally based on the selected chip.
Should be a good first step since things won't change that much if we get them from the PAC in future. Also, this way we can prepare the next steps (defining them in the SVD and having them in the PAC) without the need to rush that

@jessebraham
Copy link
Member Author

I2C support for all devices was added in #23.

@jessebraham
Copy link
Member Author

Actually I'm just going to close this issue in favour of #19.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants