Closed coelner closed 5 years ago
Hi, I've finally had some time to look into this project again. I have some questions about this merge request.
I began by installing the updated BH1750 library into my Arduino environment and then compiled and ran the simplest example, examples/BH1750test.ino. This seemed to work and I assumed was returning a int32_t even though the example creates a float lux
to hold the returned value.
I then attempted to test the new float functionality by uncommented the pre-processor section at the top of the example script. This should remove the readLightLevel function that returns a int32_t and replace it with a readLightLevel function that returns a float. I observed compilation errors. It appears that the pre-processor flags are not quite working as expected.
Compiling /path/to/BH1750/examples/BH1750test/BH1750test.ino
/var/folders/_y/fb30sc1x5l54n3cymwws4rdc0000gn/T//ccIyCBjg.ltrans0.ltrans.o: In function `loop':
/path/to/BH1750/examples/BH1750test/BH1750test.ino:51: undefined reference to `BH1750::readLightLevel(bool)'
collect2: error: ld returned 1 exit status
exit status 1
With regard to the implementation proposed, I observe you have added the function to return a float and the needed internals to support that. In addition to that you have added another function which returns an integer value. The library now appears to return either an int32_t or a float. To switch between the alternative return types you have wrapped the function implementations with the pre-processor BH1750_FLOAT flag.
What is the reason for adding the integer functionality? Why not just the float function and remove all the complexity of the pre-processor code? Alternatively, if there is a real need for the integer function why not provide it as an explicit public function name and discard the pre-processor checks. Then leave it to a user to decide which functionality they want. Personally I think just having the float function is sufficient.
When I attempt to compile the new example script that demonstrates adjusting the measurement time I observe compile errors. I see the same when I use the simple compilation check script (build-examples.bash).
$ ARDUINO_IDE_PATH=/Applications/Arduino.app/Contents/Java ./build-examples.bash
Compiling /path/to/BH1750/examples/BH1750advanced/BH1750advanced.ino
Sketch uses 5844 bytes (18%) of program storage space. Maximum is 32256 bytes.
Global variables use 427 bytes (20%) of dynamic memory, leaving 1621 bytes for local variables. Maximum is 2048 bytes.
Compiling /path/to/BH1750/examples/BH1750onetime/BH1750onetime.ino
Sketch uses 5812 bytes (18%) of program storage space. Maximum is 32256 bytes.
Global variables use 427 bytes (20%) of dynamic memory, leaving 1621 bytes for local variables. Maximum is 2048 bytes.
Compiling /path/to/BH1750/examples/BH1750test/BH1750test.ino
Sketch uses 5810 bytes (18%) of program storage space. Maximum is 32256 bytes.
Global variables use 427 bytes (20%) of dynamic memory, leaving 1621 bytes for local variables. Maximum is 2048 bytes.
Compiling /path/to/BH1750/examples/BH1750autoadjust/BH1750autoadjust.ino
/path/to/BH1750/examples/BH1750autoadjust/BH1750autoadjust.ino: In function 'void setup()':
/path/to/BH1750/examples/BH1750autoadjust/BH1750autoadjust.ino:33:14: error: 'D2' was not declared in this scope
Wire.begin(D2, D1);
^
/path/to/BH1750/examples/BH1750autoadjust/BH1750autoadjust.ino:33:18: error: 'D1' was not declared in this scope
Wire.begin(D2, D1);
^
exit status 1
I think you need to remove the references to the pins similar to the other examples.
It looks like this change would also close out #34 and #35 and #51. Is that correct?
Hi, I had some trouble with my local sync daemon and the git repo. Therefore I need time to get back into this merge request.
Thanks for your updates that add improved functionality and better refine the return values in error conditions.
I have some more review comments:
float BH1750::readLightLevel(bool maxWait)
in the BH1750.cpp
indicates two separate ranges of return values. I suspect that this comment is no longer correct and should be fixed up.int32_t BH1750::readLightLevel(bool maxWait, bool hundredth)
function? This would remove any need for having the BH1750_FLOAT setting. If you think the integer functionality is required feel then you could propose it in a separate merge request.Thank you for the hints, unfortunately I haven't finished yet.
Should be completed
sorry, I forget the example. The Readme should be clear now, the user should be aware that the sensor return only a unsigned 16 bit integer and therefore the user will get a smaller value. The datasheet announce a higher value which is only available after selecting MTreg.
Some more review comments, hopefully these are the last ones. I think it is pretty close to being good to go.
These comments are all related to the BH1750autoadjust.ino
example file.
lux = lux = readLightLevel(true);
. I suspect that this is a left over from a refactor and the extra lux was missed from being removed.loop
function so it was a little easier for me to understand. I suspect that as it was written an error would fall into the <=10 block. There also didn't seem to be a need to read the light level within each if block after changing the MTReg value. I mainly added some error handling conditions. I also removed the DEBUG comment word as this is an example where it is fine to print this state information out. Please consider refactoring it along the same lines as shown below:void loop() {
//we use here the maxWait option due fail save
float lux = lightMeter.readLightLevel(true);
Serial.print(F("Light: "));
Serial.print(lux);
Serial.println(F(" lx"));
if (lux < 0) {
Serial.println(F("[DEBUG]: Error condition detected"));
}
else {
if (lux > 40000.0) {
// reduce measurement time - needed in direct sun light
if (lightMeter.setMTreg(32)) {
Serial.println(F("Setting MTReg to low value for high light environment"));
}
else {
Serial.println(F("Error setting MTReg to default value for normal light environment"));
}
}
else {
if (lux > 10.0) {
// typical light environment
if (lightMeter.setMTreg(69)) {
Serial.println(F("Setting MTReg to default value for normal light environment"));
}
else {
Serial.println(F("Error setting MTReg to default value for normal light environment"));
}
}
else {
if (lux <= 10.0) {
//very low light environment
if (lightMeter.setMTreg(176)) {
Serial.println(F("Setting MTReg to high value for low light environment"));
}
else {
Serial.println(F("Error setting MTReg to high value for low light environment"));
}
}
}
}
}
Serial.println(F("--------------------------------------"));
delay(5000);
}
to sum it up: