rust-embedded / cortex-m

Low level access to Cortex-M processors
Apache License 2.0
819 stars 147 forks source link

peripheral: "safer" API like the one svd2rust generates? #7

Open japaric opened 7 years ago

japaric commented 7 years ago

right now you can write any u32 in the peripherals' registers. This is bad because you may write 1s to reserved bits that are supposed to always be set to 0.

svd2rust generates a "safer" API that prevents this kind of problem. But it generates the API from SVD files and I don't if there are SVD files for these core peripherals.

Should we manually implement an API like that? Or, perhaps, should we be more conservative and just provide bit masks (as consts -- think const SCB_CPACR_CP10: u32 = 0b11 << 20) to begin with? (the bit masks would save you the trouble of having to look into the documentation for the right bit offset of each bit field but they are not fool proof as you can still write to reserved bits)

cc @thejpster

thejpster commented 7 years ago

I'm OK with having bitmasks available as const to start off with.

I've sometimes wondered (when writing C) if a stronger type system would help ensure I only use the correct bitmasks with the correct registers. Perhaps newtype and judicious use of the Ops traits could achieve that. But would register |= bitmask be better that just inlining a register.set_foo(bar) function call?

japaric commented 7 years ago

I'm OK with having bitmasks available as const to start off with.

Do you know if we can get the bitmasks from somewhere like a SVD file? Extracting them manually from the reference manual sounds pretty boring.

I personally consider register |= 0b01 where 0b01 is a 2-bit bitfield (not two disjoint flags) actually evaluating to register.inner = (register.inner & !0b11) | 0b01 an abuse of the operator or at least would deem it too magical.

thejpster commented 7 years ago

Briefly:

Humm, I don't know.

It really is.

I was probably thinking of a scenario where I knew the bits in the field were currently at zero (like power on). I would fully expect the user to clear the field if required as per your example, but the important thing is that the compiler would reject the operation if you used the wrong bit field. Maybe.

On 1 Nov 2016 22:19, "Jorge Aparicio" notifications@github.com wrote:

I'm OK with having bitmasks available as const to start off with.

Do you know if we can get the bitmasks from somewhere like a SVD file? Extracting them manually from the reference manual sounds pretty boring.

I personally consider register |= 0b01 where 0b01 is a 2-bit bitfield (not two disjoint flags) actually evaluating to register.inner = (register.inner & !0b11) | 0b01 an abuse of the operator or at least would deem it too magical.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/japaric/cortex-m/issues/7#issuecomment-257716296, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6lj0nz6I_PzucSdFLTeFQQw9fEeXBiks5q57rrgaJpZM4KXwI- .

whitequark commented 7 years ago

Do you know if we can get the bitmasks from somewhere like a SVD file? Extracting them manually from the reference manual sounds pretty boring.

I've been curious about this for some time. Frustratingly and bafflingly this appears to have never been published, neither by ARM nor by any other vendor, they all write the core support code manually.

Someone did do the legwork and assemble all the system control registers in a machine-readable format (see this) but unfortunately it's not really easily convertible to SVD because there is no equivalent of the <name> field of <enumeratedValue>. It seems like an okay starting point but still an enormous amount of grunt work.

whitequark commented 7 years ago

Do you know if we can get the bitmasks from somewhere like a SVD file?

Well, and there's always the option of extracting them from the CMSIS headers with the help of a few regexps.

whitequark commented 7 years ago

Frustratingly and bafflingly this appears to have never been published, neither by ARM nor by any other vendor, they all write the core support code manually.

To my surprise, I just discovered that this is not the case. The register definitions for TM4C that I've been converting include registers and fields of NVIC, MPU, FPU, DBG and SYST (confusingly, all grouped under "NVIC"), in other words, the entire 0xE000E000 block. We are missing FPB, DWT, ITM, ETM and TPIU, but that doesn't seem like a major loss anyhow.

So we have those in SVD format, see https://github.com/m-labs/dslite2svd/blob/9ab114affd1b82938b882459a4ed0c09d97e430e/svd/tm4c129x.xml (at the very end of the file). It's not obvious that all of them should be used, or used as-is (the NVIC registers are not declared as arrays, limiting their usefulness) but I think that's an excellent starting point.

hannobraun commented 7 years ago

There are SVD files in the CMSIS 5 repository: https://github.com/ARM-software/CMSIS_5/tree/develop/Device/ARM/SVD

They seem rather incomplete, though.

whitequark commented 7 years ago

These are basically useless, I don't know why ARM even publishes them.

docbrown commented 6 years ago

There is also some discussion about this over in rust-lang-nursery/embedded-wg#46.

hannobraun commented 6 years ago

An update: @docbrown has been working on an SVD file. He doesn't plan to continue working on this in the foreseeable future, so anyone who's interested, feel free to pick this up. He's given permission to use the SVD file in any way we see fit.