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 iterable touchpad functionality #114

Merged
merged 25 commits into from
Aug 24, 2022
Merged

Add iterable touchpad functionality #114

merged 25 commits into from
Aug 24, 2022

Conversation

tekktrik
Copy link
Member

Based off of comments from #113, resolves #103

I had to fiddle with the inner working in order to be able to return Pin objects. Doesn't look like touchio.TouchIn objects store their Pin, and you can't use Pins as dictionary keys either. This adds the functionality described in the aforementioned PR, but with all the necessary tweaks to make it happen. Tested before the final pre-commit run by building as a UF2 and running the now modified example for touching all pads (which doesn't touch ALL of them anymore, for what it's worth).

@tekktrik tekktrik requested a review from a team February 18, 2022 21:44
@kattni kattni requested a review from dhalbert February 18, 2022 23:04
@kattni
Copy link
Contributor

kattni commented Feb 18, 2022

@dhalbert Please take a look at your convenience!

@tekktrik
Copy link
Member Author

If this works I can add it to the CLUE as well. Is it worth doing for the Crickit? I added an issue there as well, but can remove it if it isn't worth doing for that one.

@tekktrik
Copy link
Member Author

I realize this is a good fit for namedtuple I can refactory it to use that. Not sure how that affects memory. Would revert the _touch() based functions to use index again. Not sure how memory usage is between dict and namedtuple

@tekktrik
Copy link
Member Author

Just checking in on this PR, let me know if anything else is needed!

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

I tested this out successfully today.

I have a few small suggestions about the example code, but things are looking good to me beyond these.

I also made a build with this updated library for CPX Russian language. I believe this is one of the more space restricted ones. I did get a successful build with it.

@dhalbert you have any other thoughts on these changes?

examples/circuitplayground_touch_all.py Outdated Show resolved Hide resolved
examples/circuitplayground_touch_all.py Outdated Show resolved Hide resolved
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.

I apologize for missing the fact you reworked the original PR.

Ideally, as you mention, the way to do this would be to use board.<pin_name> as a key in a dictionary of TouchIns. Unfortunately, as you found, microcontroller.Pin is not hashable (does not implement __hash__()), so we can't do that. That would eliminate the need to use strings, and would save some getattr() calls. But I think it is fine as is.

I defer to @FoamyGuy's comments on the example.

@tekktrik tekktrik requested a review from FoamyGuy March 21, 2022 17:29
@FoamyGuy
Copy link
Contributor

I did another round of build testing for firmware size and found that the circuitplayground_express_displayio board with ru language is too large with this configuration:

Building circuitplayground_express_displayio ru

254008 bytes used, -568 bytes free in flash firmware space out of 253440 bytes (247.5kB).
12156 bytes used, 20612 bytes free in ram for stack and heap out of 32768 bytes (32.0kB).

Too little flash!!!

I'm not sure what could be removed to make room. @dhalbert or @kattni do either of you have any ideas? Does the bluefruit.py file from this repo end up getting frozen into the CPX builds? If so maybe there would be some way to leave it out as I don't think it would get used under normal circumstances on CPX.

@kattni
Copy link
Contributor

kattni commented Mar 21, 2022

We've discussed multiple times putting in a mechanism to not freeze an entire library, and this was the use case for that option. However, it has been vetoed in the end every time it has come up. I feel like it will need to be considered eventually as there are almost certainly going to be new Circuit Playgrounds. I assume the suggestion will be create a new library for the new Playgrounds. That doesn't help here as this is far too widely used to factor bluefruit into a separate library at this point.

bluefruit.py is not, in fact, used by CPX, so the concept would work. But we don't have any way to freeze partial libraries at this time. If it does not build as-is, we may need to put this on ice for now.

@dhalbert
Copy link
Contributor

The library could be refactored into a base library and separate CPX/CPB libraries. Or the CPX part could be left in the base library, since we don't freeze it into CPB.

@kattni
Copy link
Contributor

kattni commented Mar 21, 2022

Unless we can guarantee backwards compatibility, refactoring would be a bit of a nightmare. There would be so much code to update. Even if we left CPX alone.

@dhalbert dhalbert marked this pull request as draft March 21, 2022 22:44
@dhalbert
Copy link
Contributor

Changed to draft to avoid accidental merge.

@tekktrik
Copy link
Member Author

tekktrik commented May 3, 2022

Tagging adafruit/circuitpython#6342 since it's somewhat related, though it doesn't seem to be the main issue, if I understand correctly.

@tekktrik
Copy link
Member Author

Sounds like the builds may soon have more room if their counterpart isn't frozen with them. @kattni, @dhalbert - is this worth revisiting at that point?

@dhalbert
Copy link
Contributor

Yes, we can revisit this once adafruit/circuitpython#6342 i in a stable release, which would be 7.3.0 final. In addition, the work by @Neradoc here: adafruit/circuitpython#6346 to include only the parts of a frozen library that are useful (e.g. omit bluefruit.py on a CPX)) would help in terms of space.

So we can un-draft this as soon as 7.3.0 final is out.

@tekktrik tekktrik marked this pull request as ready for review June 3, 2022 00:55
@tekktrik
Copy link
Member Author

tekktrik commented Jun 3, 2022

Sounds like this can be reopened!

@tekktrik tekktrik requested a review from dhalbert June 10, 2022 13:54
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.

I've suggested a simplification (not tested!) now that it's easier to keep track of pins. The _touches dict can grow as needed: it doesn't need to contain placeholders for all the pins when it starts. Maybe you can touch up my code further.

adafruit_circuitplayground/circuit_playground_base.py Outdated Show resolved Hide resolved
adafruit_circuitplayground/circuit_playground_base.py Outdated Show resolved Hide resolved
adafruit_circuitplayground/circuit_playground_base.py Outdated Show resolved Hide resolved
adafruit_circuitplayground/circuit_playground_base.py Outdated Show resolved Hide resolved
adafruit_circuitplayground/circuit_playground_base.py Outdated Show resolved Hide resolved
@tekktrik
Copy link
Member Author

tekktrik commented Jul 9, 2022

Great suggestions! I don't immediately see any improvements to your suggestions, I think it's pretty streamlined! The code could change back towards what it was if the Pin of a TouchIn was accessible and the dict could be a list again, but since that's not the case I think this is the best solution.

@tekktrik tekktrik requested a review from dhalbert July 9, 2022 17:02
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.

Couple of simplifications.

adafruit_circuitplayground/circuit_playground_base.py Outdated Show resolved Hide resolved
adafruit_circuitplayground/circuit_playground_base.py Outdated Show resolved Hide resolved
@tekktrik tekktrik requested a review from dhalbert July 9, 2022 22:18
@tekktrik
Copy link
Member Author

tekktrik commented Jul 9, 2022

Good eye! Changes made.

@tekktrik
Copy link
Member Author

tekktrik commented Aug 19, 2022

Taking another look at this with CircuitPython 8.0.0 beta out, merged in changes from main to keep it up to date

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.

Two minor doc touchups, but looks very good! Thanks for persevering with this.

adafruit_circuitplayground/circuit_playground_base.py Outdated Show resolved Hide resolved
adafruit_circuitplayground/circuit_playground_base.py Outdated Show resolved Hide resolved
tekktrik and others added 2 commits August 24, 2022 10:22
Co-authored-by: Dan Halbert <halbert@halwitz.org>
Co-authored-by: Dan Halbert <halbert@halwitz.org>
@tekktrik tekktrik requested a review from dhalbert August 24, 2022 14:22
@tekktrik
Copy link
Member Author

@dhalbert should I go ahead and merge, or is this pending a specific CircuitPython release?

@dhalbert
Copy link
Contributor

If this works with 7.x.x, no problem. I wasn't sure whether @FoamyGuy wanted to review again.

print("Touchpads currently registering a touch:")
print(current_touched)
else:
print("No touchpads are currently regustering a touch.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Going back to test this again. noticed small typo here, regustering instead of registering

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha! Good catch.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

I tested a few variations of this again today, all were successful.

  • CPX ru language build from current core main branch with this version of the library frozen in
  • CPX 7.3.2 downloaded from cp.org with this library in the root of CIRCUITPY (to override the frozen one)
  • CPX 8.0.0-beta.0 downloaded from cp.org with this library in the root of CIRCUITPY

Looks good to me.

@FoamyGuy FoamyGuy merged commit 47f848f into adafruit:main Aug 24, 2022
@tekktrik
Copy link
Member Author

Thanks everyone!

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Aug 25, 2022
Updating https://github.com/adafruit/Adafruit_CircuitPython_BH1750 to 1.1.5 from 1.1.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_BH1750#7 from tcfranks/main
  > Use year duration range for copyright attribution
  > Keep copyright up to date in documentation

Updating https://github.com/adafruit/Adafruit_CircuitPython_BMP3XX to 1.3.15 from 1.3.14:
  > Merge pull request adafruit/Adafruit_CircuitPython_BMP3XX#22 from tcfranks/main
  > Use year duration range for copyright attribution
  > Keep copyright up to date in documentation

Updating https://github.com/adafruit/Adafruit_CircuitPython_CircuitPlayground to 5.3.0 from 5.2.8:
  > Merge pull request adafruit/Adafruit_CircuitPython_CircuitPlayground#114 from tekktrik/dev/iterable-io-rework
  > Use year duration range for copyright attribution
  > Keep copyright up to date in documentation

Updating https://github.com/adafruit/Adafruit_CircuitPython_IL91874 to 1.2.9 from 1.2.8:
  > Merge pull request adafruit/Adafruit_CircuitPython_IL91874#16 from tcfranks/main
  > Use year duration range for copyright attribution
  > Keep copyright up to date in documentation

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1322 to 1.3.6 from 1.3.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1322#17 from tcfranks/main
  > Use year duration range for copyright attribution
  > Keep copyright up to date in documentation

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1325 to 1.4.7 from 1.4.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1325#15 from tcfranks/main
  > Use year duration range for copyright attribution
  > Keep copyright up to date in documentation

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1331 to 1.3.7 from 1.3.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1331#16 from tcfranks/main
  > Use year duration range for copyright attribution
  > Keep copyright up to date in documentation

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1351 to 1.3.7 from 1.3.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1351#18 from tcfranks/main
  > Use year duration range for copyright attribution
  > Keep copyright up to date in documentation

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1608 to 1.2.17 from 1.2.16:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1608#15 from tcfranks/main
  > Use year duration range for copyright attribution
  > Keep copyright up to date in documentation

Updating https://github.com/adafruit/Adafruit_CircuitPython_TMP007 to 2.1.12 from 2.1.11:
  > Merge pull request adafruit/Adafruit_CircuitPython_TMP007#13 from tcfranks/main
  > Use year duration range for copyright attribution
  > Keep copyright up to date in documentation

Updating https://github.com/adafruit/Adafruit_CircuitPython_BLE_MIDI to 1.0.12 from 1.0.11:
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE_MIDI#10 from tcfranks/main
  > Use year duration range for copyright attribution
  > Keep copyright up to date in documentation
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
4 participants