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

Maker: Don't use pins 1, 9, 10 for captouch #20863

Merged
merged 3 commits into from Feb 27, 2018
Merged

Conversation

islemaster
Copy link
Contributor

@islemaster islemaster commented Feb 23, 2018

This is a speculative improvement to captouch. This firmata code clued us in that some of the captouch pins are used for serial communication with the host machine:

  // Tell Firmata to ignore pins that are used by the Circuit Playground hardware.
  // This MUST be called or else Firmata will 'clobber' pins like the SPI CS!
  pinConfig[1]  = PIN_MODE_IGNORE;   // Pin 1 = LIS3DH interrupt
  pinConfig[11] = PIN_MODE_IGNORE;   // Pin 11 = SPI
  pinConfig[10] = PIN_MODE_IGNORE;   // Pin 10 = SPI
  pinConfig[9]  = PIN_MODE_IGNORE;   // Pin 9  = SPI
  pinConfig[28] = PIN_MODE_IGNORE;   // Pin 28 = D8 = LIS3DH CS
  pinConfig[26] = PIN_MODE_IGNORE;   // Messes with CS too?

In particular, I theorized that enabling captouch streaming for pins 1, 9 and 10 might interfere with serial communication. If we skipped those pins when enabling captouch, the remaining pins might be more reliable.

This PR removes captouch for those pins, and all related editor configuration (autocomplete, etc). Captouch is still behind a flag so we could ship this as an experiment and see if it improves anything. Putting captouch back on these pins will be easy too.

In local testing I've seen captouch working well after this change, but I think captouch problems have always been more pronounced on other machines so I'm not sure my own testing is useful.

It's worth noting that I'm not testing against latest Firmata anymore - Adafruit just removed some of the configuration I referenced above in response to some discussion about not being able to use those pins. Apparently they can be safely shared for serial communication and other purposes. I'm still wondering if we want to allow captouch on those pins because it generates a lot more traffic than, say, attaching a button.

What do you guys think? Worth a try?

@mrjoshida
Copy link
Contributor

I think it's worth a go. My gut says that pin 1 is the real culprit, since it's shared by the accelerometer - the SPI pins shouldn't be used by anything when the firmware is not in debug mode.

@islemaster
Copy link
Contributor Author

I can easily ship a version of this that only disables pin 1, too. Any preference as to which we try first?

Copy link
Contributor

@ewjordan ewjordan left a comment

Choose a reason for hiding this comment

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

This looks fine as a code change, but...can we get on the phone or in touch with someone upstream to figure out what the longer term plan is here? Are these workarounds absolutely critical for some technical reason, or is it temporary implementation detail stuff?

@islemaster
Copy link
Contributor Author

I'll adjust to starting with pin 1 - stand by for a PR update. There's code cleanup stuff in there that's worth getting in as well, though.

Captouch isn't working on the CPX yet, it's possible that's a good opportunity to get into this conversation with adafruit. I do think we should do this speculative fix while it's behind a flag and see how far it gets us. I've also got a lead on some other board perf issues thanks to a guy in zendesk investigating some limits of Maker Toolkit - I wonder if that is also related.

@islemaster islemaster merged commit e47ec80 into staging Feb 27, 2018
@islemaster islemaster deleted the maker-limit-touch-pins branch February 27, 2018 19:50
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.

None yet

3 participants