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

MicroBit Capacitive Touch #35864

Merged
merged 18 commits into from
Jul 22, 2020
Merged

MicroBit Capacitive Touch #35864

merged 18 commits into from
Jul 22, 2020

Conversation

epeach
Copy link

@epeach epeach commented Jul 17, 2020

Implements the Capacitive Touch functionality for the MicroBit. See API spec: https://docs.google.com/document/d/1oPqJpIsaaGIQv7_KvCShmw6i0mT6MP3VqBP41yZ5M5c/edit#heading=h.ak61axyp722j

Includes tests.

Includes droplet blocks in Maker toolbox.

Should only exist behind microbit experiment flag and droplet blocks only appear when using MicroBit and not CP.

Links

Testing story

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@epeach epeach requested a review from a team July 17, 2020 16:59
super();
this.board = board;

// Flag to only trigger event on first of type
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify this comment?

this.board = board;

// Flag to only trigger event on first of type
this.connect = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I read connect as a verb. Would isConnected or hasConnected make sense here?

}

start() {
this.board.mb.streamAnalogChannel(this.board.pin); // enable temp sensor
Copy link
Contributor

Choose a reason for hiding this comment

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

temp? Should be captouch? (and below)

Copy link
Author

Choose a reason for hiding this comment

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

Ahh good catch - thanks!

this.readSensorTimer = setInterval(() => {
let currentValue = this.board.mb.analogChannel[this.board.pin];
if (this.board.mb.analogChannel[this.board.pin] !== 255) {
if (currentValue > this.releaseReading + 200 && !this.connect) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why + 200 here and + 100 below?

if (this.board.mb.analogChannel[this.board.pin] !== 255) {
if (
currentValue > this.releaseReading + this.connectedDelta &&
!this.connect
Copy link
Contributor

Choose a reason for hiding this comment

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

should now be connected?

Copy link
Contributor

@ajpal ajpal left a comment

Choose a reason for hiding this comment

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

one comment, but other than that looks great!

@epeach epeach merged commit 3dddfc2 into staging Jul 22, 2020
@epeach epeach deleted the mb-captouch branch July 22, 2020 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants