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

Indexing of touch-sensing analog groups is inconsistent #447

Open
Ecco opened this issue Apr 3, 2024 · 4 comments
Open

Indexing of touch-sensing analog groups is inconsistent #447

Ecco opened this issue Apr 3, 2024 · 4 comments

Comments

@Ecco
Copy link

Ecco commented Apr 3, 2024

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 by ST
  • In the IOSCR and IOCCR registers, with the gX_ioY bits (where X is the group number)
  • In the IOGCSR register, with the gXe and gXs bits (where X is the group number).
  • And in the output IOGxCR register, where x is the group number

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

Ecco commented Apr 3, 2024

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

Dirbaio commented Apr 3, 2024

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

Ecco commented Apr 3, 2024

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

Dirbaio commented Apr 3, 2024

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.

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

No branches or pull requests

2 participants