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

ambientlight: Add ambient light API simulator #7

Merged
merged 1 commit into from
Mar 10, 2019

Conversation

MatonAnthony
Copy link
Contributor

Add an Ambient Light API simulator. (see #5)

Signed-off-by: Anthony Maton me@anthony-maton.me

@rzr
Copy link
Owner

rzr commented Mar 9, 2019

Thx @MatonAnthony ... looks good just a remark regarding the value

maybe there is no semantic needed at the driver level...

value could just be a number to align:
https://github.com/SamsungInternet/color-sensor-js/blob/master/lib/simulator.js#L20

this should be also fixed in geolocation's simulator @Vinnl

What are you opinions on it ?

@rzr
Copy link
Owner

rzr commented Mar 10, 2019

Next step is to integrate it to lib/ambientlight/index.js and use it by default see:
https://github.com/rzr/color-sensor-js/blob/master/lib/index.js

Feel free to refactor your file if you want to align to the actual driver...

@@ -19,6 +19,7 @@

var console = require('console');
var BH1750 = require('bh1750');
Copy link
Owner

Choose a reason for hiding this comment

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

thuis could be sourounded by a try to load on your macos

Copy link
Owner

@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.

looks good could be improved now or later

try to run it
using

node lib/ambiantlight

@@ -77,6 +78,9 @@ AmbientLight.prototype.start = function start() {
this.state = 'activating';
if (!this.sensor) {
try {
if (this.options.controller === undefined) {
this.sensor = new Simulator();
}
if (this.options.controller === 'bh1750') {
Copy link
Owner

Choose a reason for hiding this comment

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

else ..

@@ -42,7 +46,7 @@ function AmbientLight(options) {

this.options = options || {};
this.options.frequency = this.options.frequency || 1;
this.options.controller = this.options.controller || 'bh1750';
this.options.controller = this.options.controller;
Copy link
Owner

Choose a reason for hiding this comment

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

|| 'simulator'

@@ -56,7 +60,7 @@ AmbientLight.prototype.update = function update() {
return self.onerror(data);
} else {
self.timestamp = new Date();
self.illuminance = Number(data);
self.illuminance = Number(data.illuminance);
Copy link
Owner

Choose a reason for hiding this comment

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

I should remove illuminance from simulator and preserve the original line to keep compatibility with bh driver

@@ -24,7 +24,7 @@ function Simulator () {
};
}

Simulator.prototype.read = function (callback) {
Simulator.prototype.readLight = function (callback) {
Copy link
Owner

Choose a reason for hiding this comment

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

ok you aligned to sensor good maybe sensor should use read, but upstream is not very reactive

please squash this change in similator.js change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intend to rebase everything into a single commit once it's ready for merge to have a clear commit easy to revert.

package.json Outdated
"keywords": [
"ambiant",
"ambiantlight",
"generic",
"i2c",
Copy link
Owner

Choose a reason for hiding this comment

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

this keyword can be kept as it can be supported

Copy link
Owner

@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.

Ok it's improving, have you tested it on macos ?

@MatonAnthony
Copy link
Contributor Author

Yes, I tested it on macos.

Copy link
Owner

@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.

now It looks good,

maybe the temp driver should not be optional as simulator is not yet done...

now you can try to squash some commits to hide intermediary works

@MatonAnthony MatonAnthony force-pushed the 5-ambientlight-simulator branch from 06dcc38 to 1e60392 Compare March 10, 2019 13:03
Add the simulator driver for ambientlight API.

- Write a simulator driver for ambientlight
- Use it as a default driver if no driver is specified

Issue-Id: rzr#5 Add simulators
Environment:
    Darwin 18.2.0
    Node 10.15.1
    Node 9.5.0

Signed-off-by: Anthony Maton <me@anthony-maton.me>
@MatonAnthony MatonAnthony force-pushed the 5-ambientlight-simulator branch from 1e60392 to 32c0ed8 Compare March 10, 2019 13:12
Copy link
Owner

@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.

all good, same should be done for temperature
then I will release a npm

@rzr rzr marked this pull request as ready for review March 10, 2019 13:21
@rzr rzr merged commit bc2a920 into rzr:master Mar 10, 2019
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.

2 participants