esp-rs / esp32-hal

A hardware abstraction layer for the esp32 written in Rust.
Apache License 2.0
192 stars 28 forks source link

Initial SVD file for ESP32 MCU. #2

Closed jeandudey closed 4 years ago

jeandudey commented 4 years ago

There are a few questions regarding the SVD file structure specially for this architecture.

To generate the definitions I followed this process:

  1. Generate definitions with: svd2rust -i esp32.svd --target none.
  2. Split the single lib.rs into modules using the form (available on crates.io) tool: form -i lib.rs -o src/ && rm lib.rs.
jeandudey commented 4 years ago

Note: the Peripherals API is unsafe by default, because there isn't a way (currently) to enter in interrupt free mode. This needs to be implemented in a separate crate, or here directly and add the proper take method to the Peripherals struct.

jeandudey commented 4 years ago

It should be ready to write a minimal HAL API to change a pin from floating mode to Output<PushPull>, and set it high/low. An example using the Peripherals API is under the examples/ directory, it's based on the MabezDev/xtensa-rust-quickstart code, sets the LED on and off.

To handle and edit the esp32.svd file (lots of repetitive work) I used the XML Notepad tool to make things much easier. It's not the best thing out there but works for this use case right now. A tool to edit SVD files would be much better but I don't know of the existence of one at the moment.

The GPIO register names, addresses, and details I used where from the ESP32 Technical Reference Manual.

MabezDev commented 4 years ago

Excellent! This is looking good. Will review properly soon.

I am wondering if there is way to generate some of this information from the existing C headers in the idf, something like https://github.com/japaric/ultrascale-plus/tree/master/tools/html2svd but C-header2svd.

HarkonenBade commented 4 years ago

Apologies that I didn't find the time I hoped to to work on this. One thing I would posit though is would it be better for the svds' and the associated generated rust to live in the 'esp32' crate repo? If only to match with the behaviour in other embedded rust environments like the stm32 one. (There stm32-rs contains the svd generated code and is pulled in by the various stm32XX-hal crates).

jeandudey commented 4 years ago

One thing I would posit though is would it be better for the svds' and the associated generated rust to live in the 'esp32' crate repo? If only to match with the behaviour in other embedded rust environments like the stm32 one. (There stm32-rs contains the svd generated code and is pulled in by the various stm32XX-hal crates).

I think the same, that having a separate repo for the esp32 definitions would be much better, and leave this repo only for esp32-hal. I have seen pattern in the nrf51-rs organization too.

jeandudey commented 4 years ago

Excellent! This is looking good. Will review properly soon.

I am wondering if there is way to generate some of this information from the existing C headers in the idf, something like https://github.com/japaric/ultrascale-plus/tree/master/tools/html2svd but C-header2svd.

As you mentioned it, it seems like a great idea and should be considered because it's an enormous effor to just create the GPIO svd definitions (and it's not complete yet with nearly ~4kLoC). Seeing the registers header files it seem to be quite parseable, especially the comments:

/* GPIO_BT_SEL : R/W ;bitpos:[31:0] ;default: x ; */
/*description: NA*/
#define GPIO_BT_SEL  0xFFFFFFFF
#define GPIO_BT_SEL_M  ((GPIO_BT_SEL_V)<<(GPIO_BT_SEL_S))
#define GPIO_BT_SEL_V  0xFFFFFFFF
#define GPIO_BT_SEL_S  0

Should be a lot of less work to go that way.

HarkonenBade commented 4 years ago

I'm going to cease leaving tidbit reviews, but can take another look later if you would be interested. My main take aways are that we have a few instances where we might get better expressiveness by constraining or renaming some of the plain high/low values for some of the registers. e.g. For the Write Set registers, writing a 0 doesnt seem to do anything according to the docs, so is it worth only enumerating the 1 values (which will mean in the rust side there is no method available to write a 0). Or for the enable registers, enumerating as enable (or enabled) vs disable, rather than high/low.

jeandudey commented 4 years ago

@HarkonenBade

I'll make some changes to the svd file, I don't know if it makes more sense for now to remove the pin descriptions in each field to have a more simpler .svd file for now (I don't know really if a header2svd could be possible right now). It makes more sense to have a single Set value on the W1TS/W1TC registers for consistence, and for the Enable/Disable values it's ok too; I used HIGH/LOW to not overcomplicate for now the .svd (it's too much error prone by hand), but makes sense after review, and for the final API. I'm going to do these changes today in a few hours or so.

MabezDev commented 4 years ago

Thanks for taking a look @HarkonenBade, I agree we should move this to esp32 repo; would you mind doing so @jeandudey?

I think we should move to automating this asap, but I am happy to merge this right away on the esp32 repo if you wish to get started on a ehal gpio driver. In the meantime I'll be looking at generating some of this via a tool, it's something I've looked into before so I have a rough idea on what to do.

Thanks @jeandudey for starting this off!

HarkonenBade commented 4 years ago

Might be worth checking out https://github.com/stm32-rs/stm32-rs if you havn't already. As we may want to use some of their SVD patching tools given that I suspect the C headers may have errors in places, or we may want to add more rich enumerated values.

jeandudey commented 4 years ago

Might be worth checking out https://github.com/stm32-rs/stm32-rs if you havn't already. As we may want to use some of their SVD patching tools given that I suspect the C headers may have errors in places, or we may want to add more rich enumerated values.

I didn't knew of that project, definitely agree on the rich enumerated values, as parsing C files isn't going to give us much information.

MabezDev commented 4 years ago

Closing as this is now unnecessary.

Thank you for getting the ball rolling!

jeandudey commented 4 years ago

@MabezDev thanks! :tada: Also I'm sorry for not being contributing now, I don't have enough time (yet).