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

add option to modify gpio base path for testing purposes #40

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sa-linetco
Copy link

No description provided.

@rzr
Copy link
Collaborator

rzr commented Jul 5, 2018

Maybe you'll be interested in:
#42

@rzr
Copy link
Collaborator

rzr commented Jul 10, 2018

Please rebase and send cleanup patches separately.

@sa-linetco
Copy link
Author

@rzr there you go :)

@rzr
Copy link
Collaborator

rzr commented Jul 11, 2018

Thx,
Please preserve current style, or send a patch that just reindent (using clang-format?) current sources and then add a feature patch on it

@EnotionZ
Copy link
Owner

From a software design perspective, I think it makes more sense to set the base directory path on the class method. On an instance level, the test option should be the filename which contains the value rather than the directory.

Also this is probably unintentional but the entire package.json file is overridden which may cause reference problems when it's publish in npm.

Copy link
Collaborator

@rzr rzr left a comment

Choose a reason for hiding this comment

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

please keep the var vs const and fix conflict then I think it can be mergeable

lib/gpio.js Outdated

var logError = function (e) {
function logError(e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this one is fine,

Copy link
Collaborator

@rzr rzr left a comment

Choose a reason for hiding this comment

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

please split this PR in several PR with smaller changes

"name": "gpio",
"version": "0.4.0",
"author": {
"name": "Sebastian Alkemade",
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to change author ? here

@rzr
Copy link
Collaborator

rzr commented Jul 26, 2018

Please try to split this change is smaller PR

@rzr rzr closed this Jul 26, 2018
@rzr rzr reopened this Jul 26, 2018
@rzr
Copy link
Collaborator

rzr commented Aug 1, 2018

Is there any interest to upstream this for next release ?

@EnotionZ EnotionZ mentioned this pull request Aug 1, 2018
@rzr
Copy link
Collaborator

rzr commented Aug 27, 2018

Maybe you can try to demonstrate this by controlling on board leds : ie

emulate actions in:
/$dir/sys/class/gpio/gpio35

would be forwarded to:
/sys/class/leds/led0/brightness

does it make sense ?

@EnotionZ EnotionZ mentioned this pull request Sep 27, 2018
@EnotionZ
Copy link
Owner

@rzr I think I see where you're getting at - it's like rather than (or in addition to) having a direct path, developers can specify a more powerful mapping. I like it, should be considered in #51.

@EnotionZ EnotionZ mentioned this pull request Sep 27, 2018
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.

3 participants