adafruit / Adafruit_VEML6070

Arduino Library for VEML6070 Digital UV sensors
14 stars 19 forks source link

Feature/multi i2c #6

Closed eecharlie closed 6 years ago

eecharlie commented 6 years ago

These changes were made to address failure scenarios if/when the Wire library causes the Arduino to hang due to bus conditions. In particular, a reproducible problem is a hang after the VEML6070's power is cycled, which is shown in unit tests.

Summary of changes: -Adafruit_VEML6070 class is now templated to use either Wire or SoftWire as I2C library -Minor edits to the vemltest example allow it to work as before -A functionally equivalent example, vemltest_softwire, was created to demonstrate initialization requirements of the SoftWire library -The unit test sketch has a single #define switch for which I2C library is used, and an additional unit test that will hang when the Wire library is used but not when SoftWire is used.

Limitations: It's still possible to get an Arduino into a reset loop under certain bad VEML6070 power conditions.

Validation: Included unit test sketch, and Travis-CI builds all worked

I get that this is rocking the boat for code that is meant to be as easy as possible for beginners. My goal is for this solution to be as widely available as possible, so let me know if there are changes that would make it possible to say yes to integrating this, if that's not already the case. The constraints are that Arduino Wire has been broken for years without adopting a solution, and Steve Marple is happy with his decisions on the SoftWire interface.

Thanks to @stevemarple for advice on the templating approach.

ladyada commented 6 years ago

hiya! we think the idea is good but we wont take the change as it is - we dont want any new dependancies especially since software i2c is rare.

instead you should pass in a TwoWire * compatible object and softwire should be a subclass or something of that. see https://github.com/adafruit/Adafruit_LIS3DH/blob/master/Adafruit_LIS3DH.cpp for an example of that

eecharlie commented 6 years ago

Ok, so I understand that the main objection is the dependency on SoftWire to compile even if it's not being used.

Supposing a software I2C that is TwoWire * compatible, is it acceptable that it needs the additional initialization commands before it gets passed in?

ladyada commented 6 years ago

yeah thats normal, with samd21 you have to create the I2C object (say, Wire1) and then pass it in as a &Wire1 reference. no templates or #define hackin' plzzzz :)

eecharlie commented 6 years ago

Ok, I'll pitch @stevemarple on this, my hands are a little tied if he doesn't buy in as the maintainer of SoftWire. Note the #define hack is only to avoid duplicating the entire unit test sketch, but I hear you that templates are a mess.

Maybe I missed it, but should I have seen subclassing going on in the LIS3DH lib? I just saw #defines to use/not use SPI.

I suppose that assuming a TwoWire* compatible I2C object, the only code changes I will then ask to be pulled would be a unit test guaranteed to fail using Wire, and possibly an example of using SoftWire instead of Wire. Is it okay for examples or unit tests to depend on a software i2c lib, so long as the library and basic example doesn't? If not, what should a resolution to #5 look like? :)

ladyada commented 6 years ago

nah there's no subclassing in lis3dh library - just that software should subclass TwoWire so you can pass in a pointer.

as for https://github.com/adafruit/Adafruit_VEML6070/issues/5, we need to solve the root cause trying to rewrite I2C will just end in support trauma :/

eecharlie commented 6 years ago

I'd suggest that the inescapable opportunity for the Arduino Wire library to get stuck in a loop forever is already causing trauma and a proliferation of alternatives :P

I may or may not get out a scope soon to see where Wire is failing with VEML6070.

You can close this pull request. Thanks for the feedback.

ladyada commented 6 years ago

veml6070 is also kinda weird, might want to find another sensor. i dont put all the blame on arduino!

eecharlie commented 6 years ago

Oh I realize it's terrible, it sometimes powers up with its configuration register in a random state for starters... I decided to contribute to the library after much suffering and back and forth with support in Germany that I wanted to save others. I recall the VEML6075 was even more poorly tested by Vishay and is complex to get a UVI value out of. And, the GUVA I2C device I recall being worse and expensive but it's been a while since I tested it. I hope someone comes out with something better, or Vishay cleans it up!

ladyada commented 6 years ago

agree - if you find a non-crummy true-UV sensor let us know. we have struggled to find a good one :/

ladyada commented 6 years ago

hiya just to follow up we're going to release a VEML6075 soon - they at least provide a algorithm for UVI - do you still want to try and find a fix for the VEML6070 issue?

eecharlie commented 6 years ago

Hi. I spent some time trying to edit @stevemarple 's library so that it could cleanly drop in as a Wire replacement, but was not ultimately successful. I saw strange errors that could have been a consequence of multiple-inheritance issues, or some memory corruption, or something I hadn't thought of. I didn't go as far as scoping I2C transactions, partly because I wasn't eager to repeat others' efforts to uncover problematic edge cases in the Wire library, knowing that I'm powerless to fix it; at best it can inform a work-around.

If you're considering any in-house testing of VEMLXXXX against more expensive and accurate sensors, I've done that and would be happy to share info that could be helpful.

ladyada commented 6 years ago

OK sure, when the VEML6075 is in the shop, that would be a cool thing to test because (in theory) it gives UV index with reasonable accuracy :)

manutremo commented 6 years ago

I have a VEML6070 in a device together with a visible and a Ir sensors. The device was hanging randomly until I disconnected the Veml, which points in the direction of the problem outlined in this thread. If I understand correctly, the cause is in the twowire lib, and even though it was identified time ago, it hasn't yet been solved so basically the VEML is unusable. Is my interpretation correct? Is there any alternative to use the VEML6070?

eecharlie commented 6 years ago

Can you try to create a reproducible example that you can post source code & wiring details for, ideally with only the VEML6070 connected? If you get to that point, I'd recommend you post in a new issue thread.

You may want to experiment with order of bus transactions or using a GPIO to toggle power to your I2C devices on a regular interval. You may not be able to selectively power off the VEML only, as it may get enough power from the I2C bus to stay alive and cause problems.

You are welcome to try my fork that uses software I2C as a work-around. You're also welcome to try getting my fork of the software I2C lib working, which would make it a drop-in Wire replacement (it currently has some strange failure cases).