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

STM32-TSC: enable discriminating between pins within same TSC group and improve TSC library in general #3274

Merged

Conversation

michelrandahl
Copy link
Contributor

@michelrandahl michelrandahl commented Aug 21, 2024

TSC Driver Updates

Introduction

This PR significantly enhances the TSC (Touch Sensing Controller) driver to improve its functionality, usability, and support for more complex touch sensing scenarios.

Key Changes

  • Add support for reading multiple channel pins from the same TSC group
  • Refactor TSC API for improved usability
  • Introduce compile-time role validation for TSC pins
  • Simplify mask creation process
  • Restructure code into multiple module files

Notable Implementation Details

  • Introduce TscAcquisitionBank to manage TSC channel pin collections
    • Purpose: STM32 TSC hardware can only read one channel per group in each acquisition
    • Acquisition banks allow efficient management of multiple channels across different groups
  • Add methods for creating and using acquisition banks
  • Implement wrapper types PinGroupWithRoles and TscIOPinWithRole for compile-time role validation
  • Modify set_io1, set_io2, set_io3, and set_io4 methods to return TscIOPinWithRole
    • Enables easy creation of acquisition banks
    • Allows use of the TscIOPin values in subsequent code logic
  • Automate mask creation based on pins defined in PinGroups
    • Eliminates need for manual mask creation by users
  • Update mask handling for selective channel reading
  • Split code into more module files

Additional Changes

  • Expand documentation
  • Add and update examples for STM32 L0, L4, F3, and U5 series
    • L0, L4, and F3 examples are designed for easy setup on Nucleo boards
      • Only basic components (appropriate resistors and capacitors) required
  • Add README files to STM32 board examples, inspired by PR Add docs to all stm32f4 examples #3077

Backward Compatibility

These changes introduce breaking changes to the current TSC implementation. However, as TSC does not seem to be part of any official embassy-stm32 release yet, I suppose that this is okay.

Testing

  • Implemented examples for STM32L0, STM32F3, and STM32L4 series
  • Tested on Nucleo-L073RZ, Nucleo-F303ZE, and Nucleo-L4R5ZI-P boards
  • Updated existing example for STM32U5, however I don't own any boards in this family to test with myself
  • tsc_multipin.rs examples demonstrates the use of new acquisition banks to read multiple TSC channel pins from the same TSC group

@michelrandahl michelrandahl force-pushed the discriminating-pins-within-tsc-group branch 7 times, most recently from 1a5fb2a to 7d93a02 Compare August 23, 2024 14:35
@michelrandahl michelrandahl force-pushed the discriminating-pins-within-tsc-group branch from 7d93a02 to f6384d3 Compare September 13, 2024 19:46
@michelrandahl michelrandahl changed the title STM32-TSC: enable discriminating between pins within same TSC group and improve TSC library ergonomics in general STM32-TSC: enable discriminating between pins within same TSC group and improve TSC library in general Sep 13, 2024
@michelrandahl michelrandahl force-pushed the discriminating-pins-within-tsc-group branch 8 times, most recently from 12d1e7d to fba76f4 Compare September 17, 2024 07:45
@michelrandahl michelrandahl marked this pull request as ready for review September 17, 2024 08:48
@michelrandahl
Copy link
Contributor Author

@Eekle, @kkoppul2, @beastbytes

I've made significant updates to the TSC driver that you've previously contributed to and might still have interest in. While you may not have approval rights for this repo, I'd appreciate any insights you might have.

@Eekle
Copy link
Contributor

Eekle commented Sep 17, 2024

The small additions I made the TSC driver are pretty much my greatest contribution to embassy :P So I don't really feel I can offer you much insight, unfortunately.

But thank you for working on this peripheral, and good luck!

@michelrandahl michelrandahl force-pushed the discriminating-pins-within-tsc-group branch from fba76f4 to c7742e7 Compare October 1, 2024 14:26
@Dirbaio
Copy link
Member

Dirbaio commented Oct 2, 2024

bender run

@Ecco
Copy link
Contributor

Ecco commented Oct 9, 2024

Is anything preventing this PR from being merged? I'd be glad to help if needed!

@michelrandahl
Copy link
Contributor Author

Is anything preventing this PR from being merged? I'd be glad to help if needed!

From my point of view its ready :)
I am also ready to comply with change requests, or get help with dealing with change requests if there might be such.

@Ecco
Copy link
Contributor

Ecco commented Oct 9, 2024

I've been trying this PR and it's great. Thanks!

I do have some feedback regarding naming : it's a bit inconsistent regarding the use of a Tsc prefix. Some elements don't use it (which I like better):

tsc::Group;
tsc::pin_groups::PinGroup;
tsc::pin_groups::G1IO1Pin;

But some elements repeat a Tsc (or tsc_) prefix (which I don't like). For example:

tsc::tsc_io_pin::TscIOPin;
tsc::TscAcquisitionBankReadings;
tsc::TscAcquisitionBank;
tsc::TscAcquisitionBankStatus;

I think it'd be better to ditch all the prefixes and rely exclusively on namespacing. This would yield:

tsc::Group;
tsc::pin_groups::PinGroup;
tsc::pin_groups::G1IO1Pin;
tsc::io_pin::IOPin;
tsc::AcquisitionBankReadings;
tsc::AcquisitionBank;
tsc::AcquisitionBankStatus;

(Note that this is not an exhaustive list).

@Ecco
Copy link
Contributor

Ecco commented Oct 9, 2024

I also have a question regarding the use of MAX_GROUP_STATUS_READ_ATTEMPTS in the examples: why is it needed? In which scenario could a reading fail only for it to succeed after a short while? Shouldn't pend_for_acquisition() implement all the required delay?

@michelrandahl michelrandahl force-pushed the discriminating-pins-within-tsc-group branch from 0a6461a to f8fd757 Compare October 10, 2024 14:23
@michelrandahl
Copy link
Contributor Author

@Ecco Thanks for the feedback! I've made the following changes:

  1. Removed all 'Tsc' and 'tsc_' prefixes. You're right, it does cut down on the noise in the code.

  2. Ditched the MAX_GROUP_STATUS_READ_ATTEMPTS check in the multipin examples. Good catch - it was leftover from an old misguided experiment and I believe such a check should not needed with async pend_for_acquisition().

I've rebased with latest main and pushed the updates as separate commits.

@dulouie
Copy link

dulouie commented Nov 9, 2024

Hi first of all thanks for this PR, I have tried the f3 blocking example with a stm32f303k8 and confirm that it works! Now I'm trying to transfer the multipin example from stm32l4 to the f3. I noticed in the description of the example that the set_active_channels() method is mentioned here, but set_active_channels_mask() is used in the code, is there a reason for this?

@michelrandahl michelrandahl force-pushed the discriminating-pins-within-tsc-group branch 2 times, most recently from 0b26d3f to 40880cf Compare November 22, 2024 15:54
@michelrandahl
Copy link
Contributor Author

Hi first of all thanks for this PR, I have tried the f3 blocking example with a stm32f303k8 and confirm that it works! Now I'm trying to transfer the multipin example from stm32l4 to the f3. I noticed in the description of the example that the set_active_channels() method is mentioned here, but set_active_channels_mask() is used in the code, is there a reason for this?

@dulouie good catch, and thanks a lot for testing! Seems this was simply a mismatch between example code and example description. Actually looking at the examples again, I realized that using set_active_channels_bank is more clear.
I have now also added a multipin example for STM32F3. However, I have created it as a 'blocking' (and not async) example since I am having trouble with STM32F3 and exti for TSC. I encountered this problem long ago #3163, but did not get time to look further into it. Its unlikely that I will be looking into that specific exti issue with STM32F3 since I most likely will be using STM32L4 for my personal projects moving forward.

I have also rebased with changes from latest main for good measure.

@michelrandahl michelrandahl force-pushed the discriminating-pins-within-tsc-group branch from 40880cf to dcd6284 Compare November 29, 2024 16:59
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thank you! 🚀

@Dirbaio Dirbaio added this pull request to the merge queue Dec 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 2, 2024
@Dirbaio Dirbaio merged commit 4acc0f8 into embassy-rs:main Dec 3, 2024
7 checks passed
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

Successfully merging this pull request may close these issues.

5 participants