H res mode2

[coelner]

to sum it up:

1. change return value to float Improve error handling #26 my own suggestion
2. check return value from Wire.requestFrom Better error handling if not two bytes are available for read. #41
3. use negative values as error message Improve error handling #26
4. added info to readme Update README to capture change to undefined light level value of 65535 #45

[claws]

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?

[coelner]

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.

1. Most of the microprocessors are bad at float calculations, but someone may find it useful to use them. I included them for testing purpose, but I did not complete this thought.
2. I'm not sure, but the pre processor flags needs to be in *.h file
3. Measurement Time Register function #34, Define conversion factor as a constant #35 and Better error handling #51 are closed
4. I try to get this ready within a week

[claws]

This comment block should be removed as it only seems possible to modify the BH1750_FLOAT setting in the BH1750.h file not in user code.

[claws]

This comment block should be removed as it only seems possible to modify the BH1750_FLOAT setting in the BH1750.h file not in user code.

[claws]

This comment block should be removed as it only seems possible to modify the BH1750_FLOAT setting in the BH1750.h file not in user code.

[claws]

Why are there two return value ranges specified in this comment? Does this comment need to be cleaned up?

[coelner]

the value within the square brackets is the maximum value if you change the MTReg value. Otherwise you will get the value within the normal brackets.

[claws]

Thanks for your updates that add improved functionality and better refine the return values in error conditions.

I have some more review comments:

1. There are a bunch of comments left in the example files that still refer to changing the BH1750_FLOAT define setting which need to be removed. The BH1750_FLOAT setting can only be modified in the `BH1750.h`` file. I'm unsure how a normal user would really modify it.
2. The comment block above the 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.
3. There is value in keeping this library simple so that it is easy to use and easy to test. The existing library returns a float value and I think we should stick with just that for now as no-one has been asking for int32_t based functionality and introducing it adds a significant amount of complexity to the code base. Can you pull out all code associated with the 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.

[coelner]

Thank you for the hints, unfortunately I haven't finished yet.

1. I fixed that.
2. that is related to the changable MTReg value. But I can add a note to that specific behaviour
3. Of course. I don't know why I implemented that feature.

[coelner]

Should be completed

[claws]

On this line the signed integer reference is no longer needed. In fact you could probably remove the first part and just say "If an error occurs...".

[claws]

The readLightLevel arguments here don't match the .h definition. There are two args when only one is expected.
Also, it s there a reason for dividing by 100? If so it would be useful for user to see a comment about why it is here.

[claws]

Same comment as above.

[claws]

Same comment as above.

[claws]

Same comment as above.

[coelner]

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.

[claws]

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.

1. Could you set the baud rate the same as the other examples? I'm not too fussed about the actual value just so long as they are all consistent. It makes testing the examples easier as I don't have to remember to switch the baud rate for just one of the example files.
2. There are multiple occurrences of lux = lux = readLightLevel(true);. I suspect that this is a left over from a refactor and the extra lux was missed from being removed.
3. I am unable to thoroughly test the example as it is currently written. If I reduce the light level seen by the board the MTReg gets set to 254 and then the program just stops running or is locked up. I'm not exactly sure what is happening here. I lower the low light MTReg value from 254 down to 100 which worked. I then incrementally worked up to a value of 176. Anything higher and I see the same lock up problem. Do you encounter a similar issue? Perhaps consider lowering the value in the example so it will likely work for most people trying out the example. I suggest something between 100 and 176.
4. If you do encounter a similar lock up issue when using large MTReg values (low light) could you add a comment in the example that large values have been observed to cause issues like locking up.
5. I restructured the example 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);
}

[coelner]

1. Done
2. Yap, done.
3. I run this sketch under esp8266 and it is working flawless. (I use the current esp8266 master branch, but I have big trouble for the last 6 month to get i2c working in a stable way. Do you use an arduino?
4. I add this comment to remind the user to test their hardware
5. I can't find any negative aspect of the refactoring. I enforced a new measurement after setting MTreg, but this is not needed if the user wait until loop() starts over. In my case I want to see the proper value immediately. In your sketch the light intensity should only change not faster than 11 seconds (Nyquist-theorem)