Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

Initial SVD file for ESP32 MCU. #2

Closed
wants to merge 2 commits into from

Conversation

jeandudey
Copy link
Contributor

@jeandudey jeandudey commented Dec 15, 2019

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

  • Is Xtensa LX6 the correct name for the cpu?.
  • What is the CPU revision? I set it to unknown for now.
  • What is the correct value for nvicPrioBits?
  • What is the correct value for vendorSystickConfig?.

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.

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

 - Is `Xtensa LX6` the correct `name` for the `cpu`?.
 - What is the CPU revision? I set it to `unknown` for now.
 - What is the correct value for `nvicPrioBits`?
 - What is the correct value for `vendorSystickConfig`?.

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)*](https://crates.io/crates/form) tool:
 `form -i lib.rs -o src/ && rm lib.rs`.

Signed-off-by: Jean Pierre Dudey <jeandudey@hotmail.com>
@jeandudey
Copy link
Contributor Author

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.

I've added a working example under the `examples/` directory,
it uses the GPIO api to set the GPIO2 (LED) on/off.

Signed-off-by: Jean Pierre Dudey <jeandudey@hotmail.com>
@jeandudey
Copy link
Contributor Author

jeandudey commented Dec 15, 2019

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.

@jeandudey jeandudey marked this pull request as ready for review December 15, 2019 16:34
@MabezDev
Copy link
Member

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
Copy link

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).

</fields>
</register>
<register>
<name>GPIO_OUT_W1TS_REG</name>

Choose a reason for hiding this comment

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

For the W1TS (and respectively the W1TC register), should we be enumerating both high and low for each pin? As from the reference document pg 62 Register 4.2, it seems that only writing 1's has any effect on the register, I'd instead propose for those registers that we only enumerate the high value, probably named as 'Set'.

Copy link

@HarkonenBade HarkonenBade Dec 17, 2019

Choose a reason for hiding this comment

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

Possibly for the W1TC register it could instead be 'Clear'.

@jeandudey
Copy link
Contributor Author

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.

<enumeratedValues>
<enumeratedValue>
<name>LOW</name>
<description>Disables pin output</description>

Choose a reason for hiding this comment

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

Should these be here just for this pin?

<enumeratedValues>
<enumeratedValue>
<name>LOW</name>
<description>Disables pin output</description>

Choose a reason for hiding this comment

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

Again description for a single pin?

<bitRange>[0:0]</bitRange>
<enumeratedValues>
<enumeratedValue>
<name>LOW

Choose a reason for hiding this comment

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

Would using say 'Enable/Disable' be more informative here than just using High/Low?

<enumeratedValues>
<enumeratedValue>
<name>LOW</name>
<description>Disables pin output</description>

Choose a reason for hiding this comment

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

Single pin description

<name>HIGH</name>
<description>Enables pin output</description>
<value>1</value>
</e

Choose a reason for hiding this comment

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

desc

@jeandudey
Copy link
Contributor Author

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.

<name>HIGH</name>
<description>Enables pin output</description>
<value>1</value>
</e

Choose a reason for hiding this comment

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

Should this not be PIN32?

@HarkonenBade
Copy link

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Member

MabezDev commented Feb 2, 2020

Closing as this is now unnecessary.

Thank you for getting the ball rolling!

@MabezDev MabezDev closed this Feb 2, 2020
@jeandudey
Copy link
Contributor Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants