Maker: Fix accelerometer #13843
Maker: Fix accelerometer #13843
Conversation
Manually deregisters Firmata's sysex response handler for Circuit Playground commands (`CP_COMMAND`) on destroy, and clears the playground-io flag that normally prevents it from registering its handler more than once per static context. This allows each new playground instance to register its own handler, which makes the accelerometer reset properly and work beyond the initial run on a page load.
In the playground-io upgrade, accelerometer.start() was removed in favor of lazily enabling streaming when an event was attached. Unfortunately we have some use cases where we want to read the accelerometer without attaching an event handler, so this change puts accelerometer.start() back.
| if (Firmata.SYSEX_RESPONSE) { | ||
| delete Firmata.SYSEX_RESPONSE[CP_COMMAND]; | ||
| } | ||
| delete Playground.hasRegisteredSysexResponse; |
There was a problem hiding this comment.
Here's the fix for problem 2: On destroy, we clear playground-io's handler for incoming data out of Firmata's static handler list, and tell playground-io that it's okay to re-register next time it's constructed. This makes me 馃槩 because it means we can't construct more than one CircuitPlaygroundBoard at a time and have them work, but in practice that's a nonissue for now.
There was a problem hiding this comment.
nit: I'd love to see a TODO comment here for the desired library improvment
|
|
||
| accelerometer.start = function () { | ||
| accelerometer.io.sysexCommand([CP_COMMAND, CP_ACCEL_STREAM_ON]); | ||
| }; |
There was a problem hiding this comment.
Restoring the start() function. See how playground-io enables accelerometer streaming when an event handler is attached. I confirmed that it's a no-op to send this command more than once.
Practical upshot is that you don't actually need to call this if you're using accelerometer events, but if you just want to read accelerometer.x or something you'll need to call start() first.
There was a problem hiding this comment.
Obviously this should move down into forked playground-io at some point (maybe all three extra methods here should) but I'd like to get this unbroken first.
| // This is a fragile approach to testing this fix, but reproducing the | ||
| // real problem in tests is going to be near-impossible since we stub | ||
| // Firmata at the webpack layer in our tests. | ||
| expect(Playground.hasRegisteredSysexResponse).to.be.undefined; |
There was a problem hiding this comment.
I'm open to suggestions on better ways to harden against this problem, given that we have to stub Firmata.
There was a problem hiding this comment.
Could we also just stub playground.handlers[CP_ACCEL_STREAM_ON] to be a Chai spy?
There was a problem hiding this comment.
I'm confused. Do you mean handlers[CP_ACCEL_READ_REPLY]? CP_ACCEL_STREAM_OUT is a command out to the board, not a message we'd expect in a sysex response; and mock-firmata really isn't set up to simulate such a response anyway.
| if (Firmata.SYSEX_RESPONSE) { | ||
| delete Firmata.SYSEX_RESPONSE[CP_COMMAND]; | ||
| } | ||
| delete Playground.hasRegisteredSysexResponse; |
There was a problem hiding this comment.
nit: I'd love to see a TODO comment here for the desired library improvment
| // This is a fragile approach to testing this fix, but reproducing the | ||
| // real problem in tests is going to be near-impossible since we stub | ||
| // Firmata at the webpack layer in our tests. | ||
| expect(Playground.hasRegisteredSysexResponse).to.be.undefined; |
There was a problem hiding this comment.
Could we also just stub playground.handlers[CP_ACCEL_STREAM_ON] to be a Chai spy?
|
As to why the libraries aren't handling this themselves... well, your guess is as good as mine. Rick opposed the addition of an |
Gets the accelerometer component working again.
Fixes two major problems:
accelerometer.start()was removed when I upgraded playground-io in Upgrade playground-io聽#12979 - obviously I didn't test that change well enough. The method was removed in favor of lazy-enabling streaming when an event handler gets attached, but in some cases we just want to read accelerometer properties, not attach events.Accelerometer events were only working on the first run after a page load. This is because only the first Playground instance was attaching a sysex-response (incoming data) handler, which in turn happens because Firmata tracks these handlers in a static context and complains if you try to replace one 馃槨 - I've already made a small change to avoid reattaching a handler but in our case we do actually want to replace it, both at the Playground and Firmata level - so I'm doing that invasively for now, and we'll look at improvements to those libraries down the road.
The latter was probably interfering with other sensors/components too, I'm not sure how much though.
Some changes I'm explicitly not addressing here, in the interest of quickly "unbreaking" the accelerometer:
accelerometer.start()in favor of blocking reads on accelerometer properties if streaming is not yet enabled.