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

bugfix/install-ubuntu #7

Merged
merged 6 commits into from
Jan 6, 2016
Merged

Conversation

lgoldstien
Copy link
Contributor

Allows installation on non-pi desktop ubuntu (to allow development of applications outside of pi land)

This merge will address parts of nebrius/raspi-io#24 nebrius/raspi-io#41

Tested npm install on:

Raspbian (2015-11-21-raspbian-jessie)
0.10.29 - system
iojs-3.3.1
4.1.1
5.1.0
5.3.0

Ubuntu 15.10
0.10.25 - system
iojs-3.3.1
0.12.7
4.1.1
5.1.0
5.3.0

This will allow the scripts/enable_i2c.js script to run on OSs that don't have this file. If /boot/config.txt already exists, it won't be overwritten.
@nebrius
Copy link
Owner

nebrius commented Jan 4, 2016

Thanks for the PR @lgoldstien! Overall this looks good, I just have one request. Instead of creating the config.txt file using touch, can you add some logic at https://github.com/nebrius/raspi-i2c/blob/master/script/enable_i2c.js#L30 instead? I think it would be cleaner and more maintainable to keep the logic for updating config.txt in one file.

You could do something like

var config;
try {
  config = fs.readFileSync('/boot/config.txt').toString();
} catch {
  config = '';
}
config = iniBuilder.parse(config, { commentDelimiter: '#' });

(note: that code wasn't tested 😅)

@lgoldstien
Copy link
Contributor Author

Thanks for the reply @nebrius good request, will implement that when I am able.

@lgoldstien
Copy link
Contributor Author

I've followed your request, and also added a warning to state that /boot/config.txt will be created if it does not exist. Retested installation on all the above platforms and works.

@nebrius
Copy link
Owner

nebrius commented Jan 6, 2016

Nice call on adding the warning, this looks great! Thanks again for the PR

nebrius added a commit that referenced this pull request Jan 6, 2016
@nebrius nebrius merged commit 63b0ec9 into nebrius:master Jan 6, 2016
@nebrius
Copy link
Owner

nebrius commented Jan 6, 2016

I just published v2.2.0 with this change!

@lgoldstien lgoldstien deleted the bugfix/install-ubuntu branch January 6, 2016 21:52
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.

None yet

2 participants