Initial touch driver #346

keirlawson · 2023-11-28T19:55:24Z

Very basic wrapper around small portion of the API surface to start with, keen to get feedback on how to evolve.

Closes #342

Vollbrecht · 2023-11-28T20:08:34Z

src/touch.rs

+ Ok(TouchDriver {})
+ }
+
+ pub fn init(&mut self) -> Result<(), EspError> {


in some drivers there is a compelling reason to split init from the creation of the driver - but is this the case for the touch driver? Same goes for calling the config step separately. I think it would be nicer to just call TouchDriver::new(&some_config)?; and be done with all initial steps. Reducing the api surface the user needs to interact on and less chance to do things out of order.

In othere words combine new / init / config into one pub api call

Yea happy to consolidated new + init + config, should the FSM stuff go in there as well? I'm not sure I quite understand what it does...

if its a onetime config that is not supposed to be changed after init, i also would combine it

hmmm in the case that the FSM mode is set to software a different start function should be used, not sure I'm understanding it right but seem that the point of this is so you can control when it starts in software, and so maybe it shouldn't be part of new? Or have I misunderstood?

Vollbrecht · 2023-11-28T20:17:18Z

Because the touch-driver only works on xtensa based esp you need to gate the complete module against the esp32 / esp32s2 /esp32s3 , otherwise its a build errror for the riscv esp's

src/lib.rs

keirlawson · 2023-11-28T20:52:32Z

Looks like esp32 has a different touch api from s2/s3, so my inclination is to restrict this PR to the later, wdyt @Vollbrecht ?

Vollbrecht · 2023-11-28T22:48:27Z

to my understanding, there is an old touch_api only for the esp32, and a newer one that works on all 3 chips. So you probably only use the newer one that can be used by all chips

keirlawson · 2023-11-28T22:54:35Z

to my understanding, there is an old touch_api only for the esp32, and a newer one that works on all 3 chips. So you probably only use the newer one that can be used by all chips

Compile against esp32 seems to be failing as the APIs are different though?

Vollbrecht · 2023-11-28T22:56:54Z

you need to use the common header api. here this header definitions is shared across all 3. There might be that there are some specific implementations that are only part of one version for example for the s3 that are not found in the othere. But you can implement the basic common set first for all

Vollbrecht · 2023-11-28T23:06:58Z

And if you use something out of a specific driver than you need to make sure to gate it to the specific esp, you can than search in the othere drivers if they have similar functions but just use them slighty different. You could try to unifiy it with one pub api and internally just use conditional compilation to get a similar feature set

Vollbrecht · 2023-11-28T23:09:04Z

as long as esp-idf-sys is not release with the touch-driver exposed you also need to use a patch.crates-io inside the Cargo.toml to point to the current esp-idf-sys master version, so CI can build it

keirlawson · 2023-11-28T23:12:34Z

Ok, I don't have an esp32 so wont be able to test any branches that are for that chip only so my concern is I'll basically be guessing as to what the right logic is in that case

keirlawson · 2023-11-28T23:17:32Z

src/touch.rs

+
+pub struct TouchConfig {
+ fsm_mode: FsmMode,
+ configured_pads: Vec<TouchPad>,


This Vec seems to be causing build issues, not sure how best to resolve?

why do you start collecting all the pads in one struct anyway. In the end from a design perspective it will be problematic, for example if you want to set or read a pin, or if you want to create an ISR/callback for it, you somehow need to be able to address that pin. So a general approach would be here similar to the PinDriver - where each PinDriver is one instance tight to one gpio pin.

If you have problems that some part of the api should only be called once like some of the init code we maybe need to split them of ( need to find that out) - in that case we would have something like a TouchDriver that would do the innit and config stuff - and then something like a TouchPadDriver for each individual pad to get to the individual get / set methods etc. Otherwise if thats not a problem just go with one struct.

So init is universal so I think we will need both, how do you envisage TouchDriver and TouchPadDriver interacting? Create the latter from a method on the former? The FSM stuff is also universal and I'm not sure if that needs to be called after conflagration of individual pads?

one thing we care about is cleaning up behind ourself automatically. So if a PadDriver goes out of scope it needs to be dropped correctly. We could make it that a TouchPadDriver gets a &TouchDriver and so it gets the lifetime of it intrinsically. Also you need to make sure to implement Drop correctly on both, so if TouchDriver gets out of scope the driver gets de-init correctly in reverse order.

Still not totally clear on what's needed here, is there analogous code in some other driver I could look at for inspiration?

For example in the SpiDriver - you can have one spi setup that service multiple connected spi-devices with for example different chip-select lines. So you are creating one overall SpiDriver and for each device you want to use it for you create an SpiDeviceDriver from a concept it would here be similar, though notice because that we have a lot of cases in spi the driver itself is somewhat complicated, so you dont need to try to understand how it works there - just how the concept works.

Ok, updated to mimic the approach of SpiDriver, lmk what you think

Vollbrecht · 2023-11-28T23:28:27Z

Ok, I don't have an esp32 so wont be able to test any branches that are for that chip only so my concern is I'll basically be guessing as to what the right logic is in that case

just go with the common api for now that is shared between all and you should be good. After that you can try to play with the specific stuff

Vollbrecht · 2023-12-03T10:13:26Z

don't forget to run cargo clippy and cargo fmt the same way you run cargo build and fix all the mentioned problems

src/touch.rs

Vollbrecht

Do you see the clippy warning in the CI for the normal esp32. The return type of the implementation on esp32 is different. You need to handle this case with a conditional compilation.
Since the touch module itself is already scoped to esp32 -esp32s3 i think the #[cfg(any(esp32, esp32s2, esp32s3))] on each codeblock could be removed since the compiler should not look here in other cases.

keirlawson · 2023-12-04T16:13:14Z

I've disabled for esp32 as I dont have the ability to test against that, so figured keeping things simpler will be easier for me as I'm new to all this

Vollbrecht · 2023-12-04T17:28:02Z

src/touch.rs

+
+impl TouchPadDriver<TouchDriver> {
+ pub fn new_single_started(pad: TouchPad, config: TouchConfig) -> Result<Self, EspError> {
+ let mut touch = TouchDriver::new(config)?;


crating the TouchDriver inside the TouchPadDriver makes little sense, ( Sorry i hope i am not rude here ;D ) You can explain your thinking here.

My thinking: As in this case you wouldn't even need all the TouchDriver buisness in the first place. Its fine to create two different version of TouchPadDriver, one with new() and one with new_single(). Where the first one takes a reference to the TouchDriver that is created before so multiple TouchPadDrivers can be created, and in the later case it takes an owned value of TouchDriver, where one would only ever use on pin.

keirlawson added 2 commits November 28, 2023 19:53

Initial impl for raw reads

0991b1a

Only enable on relevant controllers

cba54d1

Vollbrecht reviewed Nov 28, 2023

View reviewed changes

src/lib.rs Show resolved Hide resolved

keirlawson added 2 commits November 28, 2023 20:34

Gate module on correct controllers

6c61bee

consolidate initialisation

5d988a3

keirlawson requested a review from Vollbrecht November 28, 2023 20:45

remove stray import

2a5a5e5

keirlawson commented Nov 28, 2023

View reviewed changes

refactor to mimic SpiDriver

4aea49c

Add stop fn

2625c01

Vollbrecht reviewed Dec 3, 2023

View reviewed changes

src/touch.rs Outdated Show resolved Hide resolved

Vollbrecht reviewed Dec 3, 2023

View reviewed changes

src/touch.rs Outdated Show resolved Hide resolved

keirlawson added 3 commits December 3, 2023 11:19

Remove explicit stop in favour of drop

2934ed3

Unwrap errors on drop, prevent clippy warning

3e6c0b1

Fix clippy issue

b9e9727

keirlawson requested a review from Vollbrecht December 4, 2023 10:57

Vollbrecht requested changes Dec 4, 2023

View reviewed changes

Remove redundant build conditions

d5d79c7

keirlawson requested a review from Vollbrecht December 4, 2023 16:34

Vollbrecht reviewed Dec 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial touch driver #346

Initial touch driver #346

keirlawson commented Nov 28, 2023 •

edited

Vollbrecht Nov 28, 2023 •

edited

keirlawson Nov 28, 2023

Vollbrecht Nov 28, 2023 •

edited

keirlawson Nov 28, 2023

Vollbrecht commented Nov 28, 2023

keirlawson commented Nov 28, 2023

Vollbrecht commented Nov 28, 2023

keirlawson commented Nov 28, 2023

Vollbrecht commented Nov 28, 2023

Vollbrecht commented Nov 28, 2023

Vollbrecht commented Nov 28, 2023

keirlawson commented Nov 28, 2023

keirlawson Nov 28, 2023

Vollbrecht Nov 28, 2023 •

edited

keirlawson Nov 28, 2023

Vollbrecht Nov 28, 2023

keirlawson Nov 29, 2023

Vollbrecht Nov 29, 2023

keirlawson Dec 2, 2023

Vollbrecht commented Nov 28, 2023

Vollbrecht commented Dec 3, 2023

Vollbrecht left a comment

keirlawson commented Dec 4, 2023

Vollbrecht Dec 4, 2023 •

edited

Initial touch driver #346

Are you sure you want to change the base?

Initial touch driver #346

Conversation

keirlawson commented Nov 28, 2023 • edited

Vollbrecht Nov 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vollbrecht Nov 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vollbrecht commented Nov 28, 2023

keirlawson commented Nov 28, 2023

Vollbrecht commented Nov 28, 2023

keirlawson commented Nov 28, 2023

Vollbrecht commented Nov 28, 2023

Vollbrecht commented Nov 28, 2023

Vollbrecht commented Nov 28, 2023

keirlawson commented Nov 28, 2023

Choose a reason for hiding this comment

Vollbrecht Nov 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vollbrecht commented Nov 28, 2023

Vollbrecht commented Dec 3, 2023

Vollbrecht left a comment

Choose a reason for hiding this comment

keirlawson commented Dec 4, 2023

Vollbrecht Dec 4, 2023 • edited

Choose a reason for hiding this comment

keirlawson commented Nov 28, 2023 •

edited

Vollbrecht Nov 28, 2023 •

edited

Vollbrecht Nov 28, 2023 •

edited

Vollbrecht Nov 28, 2023 •

edited

Vollbrecht Dec 4, 2023 •

edited