embassy icon indicating copy to clipboard operation
embassy copied to clipboard

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

Open michelrandahl opened this issue 1 year ago • 8 comments

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 #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 avatar Aug 21 '24 15:08 michelrandahl

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

michelrandahl avatar Sep 17 '24 09:09 michelrandahl

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!

Eekle avatar Sep 17 '24 10:09 Eekle

bender run

Dirbaio avatar Oct 02 '24 16:10 Dirbaio

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

Ecco avatar Oct 09 '24 13:10 Ecco

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.

michelrandahl avatar Oct 09 '24 13:10 michelrandahl

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 avatar Oct 09 '24 20:10 Ecco

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?

Ecco avatar Oct 09 '24 22:10 Ecco

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

michelrandahl avatar Oct 10 '24 14:10 michelrandahl

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 avatar Nov 09 '24 13:11 dulouie

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 https://github.com/embassy-rs/embassy/pull/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 avatar Nov 22 '24 16:11 michelrandahl