Skip to content
This repository was archived by the owner on Sep 30, 2019. It is now read-only.

Conversation

@IainColledge
Copy link

Pulled together a working and up to date python driver for the TSL2561 Lux sensor.

Has been compared to a 10 UKP Lux meter and was within 10% across the range.

@csatt
Copy link

csatt commented Dec 10, 2015

Hi Iain,

Thanks for doing this - it's just what I was looking for!

I have played with your code, and there are a couple bugs and some other
more cosmetic issues. I posted the same list on the Adafruit forum thread.

Bug #1: The class name has the middle two digits transposed

    Adafruit_TSL2651 should be Adafruit_TSL2561

Bug #2: The read8 and read16 methods (functions) call the I2C readS8 and
readS16 methods respectively. They should call the readU8 and
readU16 (i.e. unsigned) methods.

    ==> This is why you needed the hack that you added. The readS16
        method was returning a negative number when the register had
        its upper bit set. You can remove the hack when this bug is
        fixed.

Other minor issues:

  • Adafruit_I2C.py should not be a file; it should be a symbolic link to
    ../Adafruit_I2C/Adafruit_I2C.py
  • The trailing */ on comments should be removed (vestige of C++ code)
  • Add space char after "=" in constant definitions at the beginning of
    the class
  • In init constructor, use TSL2561_ADDR_FLOAT instead of 0x39
  • You changed "enableAutoRange" to "enableAutoGain". I prefer the new
    name but perhaps in the interest of keeping the port from the C++
    code as pure as possible it should keep the old name.
  • The code in enableAutoGain is odd. Couldn't it be just one line? i.e.:
    self._tsl2561AutoGain = enable
  • Some methods have debug printing xxx_end after the return statement
    (e.g. read8)
  • It would probably be better to put the comment preceding each method
    into a docstring at the beginning of the method definition (low
    priority)
  • Python purists wouldn't like all the unnecessary parentheses used in
    if, elif, while statements (it doesn't bother me though)

@IainColledge
Copy link
Author

Thanks very much, will update and do another pull request, really appreciated.

@IainColledge
Copy link
Author

Tidied up the code so it looks like Python now, just adding some things like throwing an exception for saturation and adding handlers to calling code.

@IainColledge
Copy link
Author

This is about it for now as don't have the time at moment to go into the deeper issues.

- Changed Adafruit_I2C.py from a copy to a symbolic link
- Added underscore back into class name
- Removed unnecessary inheritance from Adafruit_I2C
- Removed vestigial trailing */ from comments
- Removed (now unnecessary) autogain hack
- Fold (most) long lines to comply with col 80 limit
- Added BSD license header comment
Add the underscore to the class name in Adafruit_TSL2561_example.py
@csatt
Copy link

csatt commented Dec 18, 2015

Hi Iain,

I forked from the latest version of your fork, and made several
changes. I am new to GitHub, but I believe that I can now submit a pull
request and it will go to you and not to the Adafruit repo owner, since
my fork is off of your fork. We'll see!

You should be able to see the diffs in the pull request. A few of these
are things that I mentioned in my feedback to you on your original
version. Let me know if there are any you disagree with and I can make
changes and re-submit the pull request. Here's the summary:

- Changed Adafruit_I2C.py from a copy of
  ../Adafruit_I2C/Adafruit_I2C.py to a symbolic link

  I'm sure your pull request would be rejected without this change.

- Added underscore back into class name

  You were probably trying to conform to a coding standard you read
  about. But I think it is more important to conform to the naming
  conventions that are already used in the Adafruit library.
  Unfortunately there are two such conventions. The class name
  should either be Adafruit_TSL2561 or just TSL2561. I prefer the
  former since it matches the name of the file.

- Removed unnecessary inheritance from Adafruit_I2C

  You had a comment wondering if this inheritance was
  necessary. It's not. I removed the comment and changed it to the
  generic "object" inheritance.

- Removed vestigial trailing */ from comments

  I'd mentioned this in my earlier comments.

- Removed (now unnecessary) autogain hack

  The hack was only necessary when you had the signed/unsigned bug.
  Now it works fine without the hack, so I removed it.

- Fold (most) long lines to comply with col 80 limit

  This is just a cosmetic change, but better made now than
  later. The 80-column convention has archaic roots (punch cards!),
  but still is nice so the code is readable in a narrow window
  (expecially when doing a side-by-side diff).

- Enhanced the saturation exception so the message text says which
  channel (or both) was saturated

- Added BSD license header comment

  In theory, all code in the Adafruit repository should have this
  boilerplate header. Having it increases your chances of getting
  the pull request accepted.

You also had the following comment before the exception code:

"TODO: Fix that exception not raised when hits saturation but returns a Lux of around 780"

I removed that comment because I did not observe that problem. I tested
it in all of the different modes and was able to get the saturation
exception in all cases (broadband only).

  • Chris

P.S. I am going to be gone for the next 5 days. I may see e-mail but
won't be able to make (or more importantly, test) any code changes.

@IainColledge
Copy link
Author

Chris, thanks very much, I've merged your pull request and afaik given you permissions to merge anything into this pull request in the future, you have write permission to my fork and this pull.

I'll try the saturation test again which is basically holding a maglite above the sensor until it max's out to see if the hack is needed for me or not. Will let you know what happens.

The coding standards were basically from PEP 8 and suing Pycharm & SonarQube but meh am not fussed, it at least looks Pythonish vs C++ now ;)

Have a great 5 days away and if I don't hear from you a great x-mas, I'll be available until about the 5th. Would be good to put this sucker to bed.

@csatt
Copy link

csatt commented Dec 19, 2015

Hi Iain,

Looks like the fork-of-a-fork thing worked. Good to know! Thanks for giving me push permission. That's something else I haven't done before, but I'm sure it's straightforward if I need to use it.

It will be interesting to hear what you find when you try the saturation test again. Your test sounds pretty similar to mine. It's easiest to hit the saturation point in the 402ms integration time mode because it happens at a lower lux value, but I was able to hit it in all three modes if I got the light close enough. The exception seemed to work fine.

I'm not likely to contribute much more to this since my project is working now, and it was just a personal project anyway. I think the code is in good shape and I hope your pull request is granted.

I hope you have a great Christmas as well!

  • Chris

@IainColledge
Copy link
Author

Will let you know and thanks, will update my code for the dataloop.io interface to use the new class name.

Iain

@ladyada
Copy link
Member

ladyada commented Sep 30, 2019

Thank you for the Pull Request
This library has been deprecated in favor of our python3 Blinka library. We have replaced all of the libraries that use this repo with CircuitPython libraries that are Python3 compatible, and support a wide variety of single board/linux computers!

Visit https://circuitpython.org/blinka for more information

CircuitPython has support for almost 200 different drivers, and a as well as FT232H support for Mac/Win/Linux!

@ladyada ladyada closed this Sep 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants