Pin resolutions by monteslu · Pull Request #144 · firmata/firmata.js

monteslu · 2016-12-04T05:53:13Z

This adds resolution or label properties to pins.

Also adds a new test and a few more assertions

Fixes #105

soundanalogous · 2016-12-04T07:51:17Z

      if (board.currentBuffer[i] === 127) {
-        board.pins.push({
-          supportedModes: supportedModes(capability),
-          mode: undefined,


the mode property was not carried over

soundanalogous · 2016-12-04T08:07:13Z

LGTM. I'll wait for @rwaldron to weigh-in in case he has any nits.

rwaldron · 2016-12-04T14:52:56Z

-          value: 0,
-          report: 1,
-        });
+        pin.supportedModes = supportedModes(capability);


var pin = {}; should be above this line.

Problem is that the 127 comes at the end of the pin definition in the byte stream. I need to attach the new properties before that.

Ok, I see now. I still think this should be refactored, to avoid this strange pin binding reassignment, but I think we can come back to it later (ie. not this PR).

Added the comment for clarity on byte 127. Save refactor for ES2015+ in firmata v 3?

@soundanalogous @rwaldron Are we good for now? I think I don't really have anything else to do if we're going to refactor this as ES2015+ in a later version.

I'm good with the pin resolution changes, however I've been rethinking how to represent the Serial pins. I need to think more about how that information would be used (since you don't really need to think about pins when using a UART - you just need to know if it's Serial1, Serial2, Serial3, etc). One option is to revert the Serial pin changes for now and revisit once I have a better idea how to use them.

rwaldron · 2016-12-04T14:53:05Z

+        pin.value = 0;
+        pin.report = 1;
+        board.pins.push(pin);
+        pin = {};


Remove this line

soundanalogous · 2016-12-04T18:29:55Z

  if (!board.pins.length) {
+    var pin = {};
    for (var i = 2, n = 0; i < board.currentBuffer.length - 1; i++) {
      if (board.currentBuffer[i] === 127) {


Add the following comment above this line:
// 127 denotes the end of the pin's modes

soundanalogous · 2016-12-04T18:40:41Z

        continue;
      }
      if (n === 0) {
        capability |= (1 << board.currentBuffer[i]);


You could initialize the pin object here.

although it's probably not good practice to initialize a variable in a block where that variable is also used out side that block (even though it works with var)

soundanalogous · 2016-12-04T19:03:20Z

      }
      if (n === 0) {
        capability |= (1 << board.currentBuffer[i]);
+        pin[modeNames[board.currentBuffer[i]] + "Resolution"] =  board.currentBuffer[i + 1];


Maybe a better strategy is to not apply these properties directly to the pin object here. Apply them to a temporary object and then apply to pin only in the above block. That way all assignment occurs within the same block much like how capability is applied to the pin in the above block as well.

monteslu · 2016-12-04T19:43:16Z

Sounds good. Kinda wish we had Object.assign though. I'll do a for(in...

soundanalogous · 2016-12-04T21:12:44Z

What is the state of ES2015 support in Node? Perhaps worth migrating firmata.js at some point in the near future?

soundanalogous · 2016-12-04T21:28:17Z

Looks like johnny-five uses Object.assign. Could add the es6-shim module and then define something like this:

(!Array.from || !Object.assign || !Map) {
  require("es6-shim");
}

rwaldron · 2017-01-12T19:37:41Z

What is the state of ES2015 support in Node? Perhaps worth migrating firmata.js at some point in the near future?

I've had a WIP branch for about year, I suppose I could revive it

soundanalogous · 2017-01-12T19:53:28Z

Here's what I think the next steps are here:

Undo the changes related to the Serial pin labels, I need to think more about how to handle that.
Add an array of pin modes that have resolution values (to use as a comparison so all pins modes don't end up with "<modeName>Resolution". I think we really only care about analog in (analogResolution) and analog out (pwmResolution) for now (for modes like Servo and Stepper, I'd rather add commands to query the resolution for those features directly as opposed to returning via capability query - I want to trim down the capability query response size for Firmata 3.0).

rwaldron · 2017-01-12T20:16:44Z

I'm presently working out how to make these consistent for all platforms and IO plugins that model their exposed API after Firmata.js

rwaldron · 2017-01-12T20:44:57Z

(to use as a comparison so all pins modes don't end up with "Resolution"

This matches the path I'm presently on as well.

I think we really only care about analog in (analogResolution) and analog out (pwmResolution) for now

I agree

rwaldron · 2017-01-12T21:23:24Z

@soundanalogous how likely is it that a given board will have more than one resolution for a specific "kind" of thing? I cannot recall ever encountering a board that had, for example, two different ADC chips, where one was 10-bit and the other 12-bit. Just so there's no misunderstanding, it is common to see ADC X-bit, PWM Y-bit and DAC Z-bit.

soundanalogous · 2017-01-12T21:25:08Z

Not that I'm aware of, but I also haven't looked into it in detail for a while. At least with Arduino, on some boards you can change between 2 analog read or write resolutions, but this is global.

rwaldron · 2017-01-12T21:41:11Z

Not that I'm aware of, but I also haven't looked into it in detail for a while

I've looked through all of the boards that are currently supported via the "io-plugin" interface and none have such a concept. I'm sure there is some silly Intel board somewhere that does that, but I'm not going to worry about it. Given that, I've refactored as such 6c0e0fa In most other platforms, implementation will be as trivial as adding the properties with hard values. Then consumers of Firmata.js instances can access the value as board.RESOLUTION.FOO, just as they would board.MODES.FOO

soundanalogous · 2017-01-12T22:07:59Z

I think the null check is okay for now, but at some point I'm going to add support for the user to change the analog resolution value: firmata/protocol#8. I guess we can just change the logic when that lands (not likely for a while).

rwaldron · 2017-01-12T22:08:44Z

That's awesome :) Yes, I think it will be relatively easy to adapt this.

monteslu · 2017-01-16T18:31:46Z

This was resolved with #149

monteslu added 4 commits December 1, 2016 20:55

pin resoloution naming based on MODE map keys

0226ec0

update readme

2665152

pin labels for serial

e089e37

add label to readme

053c99f

soundanalogous reviewed Dec 4, 2016

View reviewed changes

mode property back into pin object

ef65ead

rwaldron reviewed Dec 4, 2016

View reviewed changes

Comment thread lib/firmata.js

pin.value = 0;

pin.report = 1;

board.pins.push(pin);

pin = {};

Copy link
Copy Markdown

Collaborator

rwaldron Dec 4, 2016

Choose a reason for hiding this comment

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

Remove this line

soundanalogous reviewed Dec 4, 2016

View reviewed changes

add comment to note pin modes finished in stream

84db4e7

soundanalogous mentioned this pull request Jan 12, 2017

Support analog (and motor) control on the ESP8266 firmata/arduino#344

Closed

rwaldron mentioned this pull request Jan 12, 2017

Expose bit resolution of ADC, PWM (and others as available) #149

Merged

monteslu mentioned this pull request Jan 14, 2017

Feather HUZZAH (ESP8266), generic H-bridge motor drivers, and PWM range rwaldron/johnny-five#1275

Closed

monteslu closed this Jan 16, 2017

Conversation

monteslu commented Dec 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soundanalogous commented Dec 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monteslu commented Dec 4, 2016

Uh oh!

soundanalogous commented Dec 4, 2016

Uh oh!

soundanalogous commented Dec 4, 2016

Uh oh!

rwaldron commented Jan 12, 2017

Uh oh!

soundanalogous commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwaldron commented Jan 12, 2017

Uh oh!

rwaldron commented Jan 12, 2017

Uh oh!

rwaldron commented Jan 12, 2017

Uh oh!

soundanalogous commented Jan 12, 2017

Uh oh!

rwaldron commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soundanalogous commented Jan 12, 2017

Uh oh!

rwaldron commented Jan 12, 2017

Uh oh!

monteslu commented Jan 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

monteslu commented Dec 4, 2016 •

edited

Loading

soundanalogous commented Jan 12, 2017 •

edited

Loading

rwaldron commented Jan 12, 2017 •

edited

Loading