embassy-rs / stm32-data

69 stars 101 forks source link

Indexing of touch-sensing analog groups is inconsistent #447

Open Ecco opened 5 months ago

Ecco commented 5 months ago

Depending on the specific STM32 device, the Touch-Sensing Controller (TSC) supports up to 8 analog groups. These groups are labeled 1 through 8, in multiple places:

In the reference manual and in stm32-metapac, the convention is that groups are named 1 through 6 (or 8, depending on the device). However, stm32-metapac exposes an (otherwise convenient) function iogcr(n) to retrieve the IOGxCR register of a given group, and this specific function uses a 0-based index. This is very inconsistent. For example, to enable then read the first analog group, one would currently need to write:

TSC.iogcsr().modify(|v|{
  v.set_g1e(true);
}

TSC.iogcr(0).read().cnt();

Notice how we have to use the value of 1 on the first block and 0 in the second.

Ecco commented 5 months ago

I would suggest making the iogcr() function use a 1-based index. Maybe a patch like this would work:

data/registers/tsc_v1.yaml
  - name: IOGCR
    description: I/O group x counter register.
    array:
-      len: 6
+      len: 7
      stride: 4
-    byte_offset: 52
+    byte_offset: 48

Might not be the cleanest though (it wouldn't yield an error on iogcr(0)), but at least it's easy to implement. If this is something you'd be interested in, I'd be happy to provide a PR.

Dirbaio commented 5 months ago

Actual indices (handled in the code as usizes) should be always 0-based, otherwise usage becomes much more complicated. For example if the user creates an array to store state per channel they either have to waste RAM for the unused 0th index, or remember to add/subtract 1 every time.

ST loves to use 1-based names, we already live with this inconsistency in many places. For example "DMA channel 1" is index 0, etc.

In this particular case, it seems to me all the gX_ioY, gXe, gXs bits could (should!) be turned into arrays, making them 0-based, so there's no inconsistency anymore within the PAC.

Ecco commented 5 months ago

Smart. Indeed I don't mind too much the ST/PAC inconsistency. I do mind the inconsistency within the PAC though.

I think I might be able figure out on my own how to convert the gXe and gXs bits into arrays, but would you have any pointers on how to convert the gX_ioY bits into two-dimensional arrays?

Dirbaio commented 5 months ago

into two-dimensional arrays?

ah, indeed this is not supported in chiptool :( you can do it for registers by using blocks, but you can't do it for bits within a fieldset.

we could add it, or we could arrayify only one dimension and just rename the fields in the other to be 0-based.