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

Add iteration support for touchpads #113

Closed
wants to merge 19 commits into from
Closed

Add iteration support for touchpads #113

wants to merge 19 commits into from

Conversation

tekktrik
Copy link
Member

@tekktrik tekktrik commented Feb 1, 2022

Resolves #103 as follows:

  1. Adds touched property for showing list of of pin names currently registering a touch
  2. Adds touchpads property got showing list of pin names that are set up as touchpad inputs
  3. Continues to allow inputs to only be initialized as touchio.TouchIn as needed
  4. Now allows for touchpads to be initialized and deinitialized as touchpad inputs, which allows those pins to be used for other things if wanted!
  5. Existing methods for checking touchpads still works!
  6. Modifies/adds examples to demonstrate new functionality

This is all powered by the new IterableInput class, which should remain hidden from the user but provides the above functionalities.

@tekktrik tekktrik requested review from kattni and a team February 11, 2022 15:12
Copy link
Contributor

@kattni kattni left a comment

Choose a reason for hiding this comment

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

My concern here is increasing the size of the library. We need to make sure it still fits frozen in CircuitPython for the CPX before considering merging this.

@tekktrik
Copy link
Member Author

Sounds good, I can look at removing anything not immediately needed as well. If any of the above functionalities aren't needed, let me know an I can try and factor them out. Also, if there's a good way to test this library as frozen.

@kattni
Copy link
Contributor

kattni commented Feb 17, 2022

@tekktrik There absolutely is a way to test it! That's the next step before trying to slim down the code. Do you know how to build CircuitPython? Testing a frozen library is pretty simple once you have a CircuitPython build environment set up.

@tekktrik
Copy link
Member Author

No, I don't actually, I think I've seen guides though, is there a Learn guide for it?

@kattni
Copy link
Contributor

kattni commented Feb 17, 2022

Yep! It's here. Please message me on Discord if you need assistance. Once you have the build environment working, I'll show you how to test the frozen library.

@tekktrik
Copy link
Member Author

tekktrik commented Feb 17, 2022

So it builds, and it currently doesn't look like I have too much in the way of "unused" code. So I think it really depends on what functionalities are worth introducing and just cutting the rest:

  1. touched property for showing pin names (str) currently registering touch
  2. touchpads property shows the pins names (str) that are currently setup as touch inputs
  3. It only initializes touch inputs as needed, similar to how it's been done before this
  4. You can init and deinit touchpads. This seemed like a big want, but I don't know the balance of freezing this into the firmware vs trying to recover the pins as touchpads or saving RAM
  5. You can access pins by both pin names (str) and pin numbers (int)

Let me know what to cutout!

@tekktrik
Copy link
Member Author

Also, it may make sense to hide this class in the documentation, let me know if I should exclude it in api.rst

@dhalbert
Copy link
Contributor

I started to do a review, but will start at the higher level. The main motivation for this was two things:

  1. return a sequence of touched pads
  2. return the touchpads currently in use.
  • You can init and deinit touchpads. This seemed like a big want, but I don't know the balance of freezing this into the firmware vs trying to recover the pins as touchpads or saving RAM

I cannot think of a practical use case for this . If a pin is being used as a touchpad, you would not in general want to connect anything else up to it. Electrically, that would interfere with its use as as touchpad. In a single program, you there is not a common need to deinit and reinit a touchpad. So I'd say just leave this out. We still need create TouchIn objets dynamically, as was true before.

  • You can access pins by both pin names (str) and pin numbers (int)

I don't think it is a good idea to have multiple ways to name a pin. We already have a good way, which is the board pin name. It's true that is not used in this library, but we can introduce it. If we introduce naming pins by strings or integers, that will be unique to this library, and will be confusing to the beginner, who will think that you might be able to do the same with other modules or libraries, such as digitalio.DigitalInOut("A1") or busio.I2C(3, 4), which we don't allow and don't want to allow.

So I suggest:

  • Add a touched property which returns a sequence of pins that are currently touched: [board.A2, board.TX], for example. The user will have to import board to get pins to compare against these, and so an example should show that.
  • Add a touch_pins property which returns a sequence of pins that are currently in use as touchpads: board.A2, board.A3, board.TX], for example.

I think that will cover the existing issue. You can use the existing ._touches list and just iterate over it for both of the above properties, and probably even write it as a comprehension. I think something like this code will work, and it's small:

    @property
    def touch_pins(self):
        return [touch for touch in self._touches if isinstance(touch, touchio.TouchIn)]

    @property
    def touched(self):
        return [touche for touch in self.touch_pins if touch.value]

Copy link
Contributor

@dhalbert dhalbert 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 for working on this. See main comment in comment thread.

if value in (7, "A7", "TX"):
return "TX"
if isinstance(value, int):
if value not in range(1, 7):
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is no longer needed given my general comment, but note that range(1, 7) will allocate storage. Also, the upper limit is exclusive and is not included; it will only go from 1 to 6.

Suggested change
if value not in range(1, 7):
if not (1 <= value <= 7):

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh neat, I didn't think about that, thanks!

@tekktrik
Copy link
Member Author

Thanks for reviewing! Let me take a stab at this, but I may end up just creating a new branch for this if it makes more sense to start from scratch :)

@kattni
Copy link
Contributor

kattni commented Feb 18, 2022

@tekktrik Starting a new PR is perfectly fine. Simply make sure you reference this PR in the new PR notes so there's some continuity there. You can leave this open until you create the new one, if you like.

@tekktrik
Copy link
Member Author

Closing in favor of #114

@tekktrik tekktrik closed this Feb 18, 2022
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.

Capacitive touch iterable attribute
3 participants