Skip to content

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

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

Merged
merged 2 commits into from
Jan 16, 2017

Conversation

rwaldron
Copy link
Collaborator

@rwaldron rwaldron commented Jan 12, 2017

Simplified mechanism for exposing resolution values from capabilities response. Intended to supersede #144

Signed-off-by: Rick Waldron [email protected]

@@ -505,6 +523,12 @@ function Board(port, options, callback) {
RES_TX3: 0x07,
};

this.RESOLUTION = {
ADC: null,
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the user skips the configurationQuery? Do we need defaults values set here instead?

Copy link
Member

Choose a reason for hiding this comment

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

I guess another way to think about this is do the default values live in firmata.js or in johnny-five (where the normalization of analog and pwm values takes place).

Copy link
Member

Choose a reason for hiding this comment

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

This is the only place in firmata.js where there is a reference to the resolution: https://github.com/firmata/firmata.js/blob/master/lib/firmata.js#L740. If we set defaults in firmata.js that param description could change to something like:

"The data to write to the pin between 0 and this.RESOLUTION.PWM."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What happens if the user skips the configurationQuery? Do we need defaults values set here instead?

That's reasonable, we should just use Arduino defaults. (update shortly)

If we set defaults in firmata.js that param description could change to something like:
"The data to write to the pin between 0 and this.RESOLUTION.PWM."

Sounds good

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this better than my stab at it :)

👍

@coveralls
Copy link

coveralls commented Jan 13, 2017

Coverage Status

Coverage increased (+0.03%) to 96.694% when pulling bb46a89 on resolution-alternative into 5710163 on master.

Signed-off-by: Rick Waldron <[email protected]>
@rwaldron rwaldron force-pushed the resolution-alternative branch from 36b37c7 to 6c1ec84 Compare January 16, 2017 14:58
@coveralls
Copy link

coveralls commented Jan 16, 2017

Coverage Status

Coverage increased (+0.1%) to 96.784% when pulling 61ae483 on resolution-alternative into 5710163 on master.

@coveralls
Copy link

coveralls commented Jan 16, 2017

Coverage Status

Coverage increased (+0.04%) to 96.703% when pulling 6c1ec84 on resolution-alternative into 5710163 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 96.703% when pulling 6c1ec84 on resolution-alternative into 5710163 on master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 96.703% when pulling 6c1ec84 on resolution-alternative into 5710163 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 96.703% when pulling 6c1ec84 on resolution-alternative into 5710163 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 96.703% when pulling 6c1ec84 on resolution-alternative into 5710163 on master.

@coveralls
Copy link

coveralls commented Jan 16, 2017

Coverage Status

Coverage increased (+0.04%) to 96.703% when pulling 6c1ec84 on resolution-alternative into 5710163 on master.

@soundanalogous
Copy link
Member

LGTM

@rwaldron
Copy link
Collaborator Author

@soundanalogous @monteslu thanks for reviewing

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.

4 participants