feat: Added VL531X Time-of-Flight Proximity Sensor #228
Conversation
Reviewer's Guide by SourceryThis pull request adds support for the VL531X Time-of-Flight Proximity Sensor. The implementation includes a new VL531X class that inherits from I2CSlave, facilitating easy initialization and integration with the existing I2C API. The changes also update the import statements in 'supported.py' and add the VL531X sensor to the supported dictionary with its I2C address. The new class includes methods for sensor initialization, configuration, and data reading, ensuring compatibility with the current I2C API. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @AsCress - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider breaking up the VL531X.py file into smaller, more manageable parts. For example, you could move the configuration constants to a separate file.
- Great job on implementing the VL531X sensor support. Please create separate issues to track the TODO comments in the code for future improvements.
Here's what I looked at during the review
- 🟡 General issues: 13 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
AsCress
force-pushed
the
development
branch
2 times, most recently
from
c15101f to
13efe68
Compare
|
If #130 is used a base, please make your changes on top of those commits, not rebased on development. orangecms should be credited for the work he did.
Ideally, the TODOs should be dealt with by answering the questions they pose. Take That's just one example. If the questions remain unanswered, the TODOs should also remain. To be honest, I don't like this code. I don't understand it, even using the datasheet as a reference. Which is why #130 was not merged in the first place. There exist drivers for this sensor which do not rely (as heavily) on writing magic numbers to undocumented registers, such as: https://github.com/embvm-drivers/ST-VL53L1X I would prefer an implementation adapted from such code. But I recognize that would be a lot of work. Alternatively, we could use Adafruit's circuitpython driver (which also suffers from magic number-itis, but at least then it's not OUR magic numbers) via pslab-python's circuitpython busio compatibility layer. It should be as simple as: import adafruit_vl53l1x
from pslab.bus.busio import I2C
i2c = I2C()
vl53 = adafruit_vl53l1x.VL53L1X(i2c)
vl53.start_ranging()
while True:
if vl53.data_ready:
print("Distance: {} cm".format(vl53.distance))
vl53.clear_interrupt()
time.sleep(1.0)Could you try and see if the above snippet works? To summarize the above, I see three approaches, which I've ordered here from my most to least favorite:
If option # 2 does not work well, I can be persuaded to go for # 3. |
|
@bessman Thank you very much for your comments !! I just want to mention a few things:
It gives this output and the sensor works absolutely fine: My views on this are that since we have a compatibility layer which works nicely, we shouldn't add sensors manually to our repository any more. We already have access to a whole big lot of sensors. |
Noted. I wasn't sure which of 0X or 1X was the target or what the difference between them is.
Awesome! Which firmware version is this? You mentioned that you were having problems with I2C in the latest firmware.
Yes, I agree. There may be advantages to rolling our own driver in special cases, e.g. if it's a sensor that is very popular to use in conjunction with the pslab, or if the circuitpython driver doesn't work well. In this case, I think we should prefer the circuitpython driver. Are you OK to close this PR in favor of using the circuitpython driver? |
The busio compatibility layer works nicely on both the legacy and the latest firmware version, referring to our recent discussion.
True indeed. We can always observe which sensors are used the most and possibly write our own libraries for them.
Absolutely ! The circuitpython drivers are the way ahead for all the sensors which Adafruit supports currently:)) |
Superseeds #130, adds support for the VL531X Time-of-Flight Proximity Sensor.
I've taken #130 and made changes to update the I2C calls, making them compatible with our current I2C API.
Other suggested changes in #130 are also implemented.
All credits to @orangecms for the functionality and the port from Adafruit's implementation.
Changes
VL531Xis updated to inherit fromI2CSlave, like all the other available sensors, facilitating easy initialization (sensor = VL531X()).read_bulk) are changed to slave methods (such asread,write, etc.) and other necessary changes are made.Screenshots / Recordings
I've tested this and it works nicely. Here's how one can get data from the sensor -
@bessman I see a couple of TODOs and questions in the comments. How should we go about them ? Should we just let them be there and merge this or should we document this more ?
Do inform me if I haven't properly followed conventions in here:))
Summary by Sourcery
This pull request introduces support for the VL531X Time-of-Flight Proximity Sensor by adding a new class
VL531Xthat inherits fromI2CSlave. The I2C calls have been updated to be compatible with the current I2C API, and the class has been refactored to use slave methods. The sensor is now included in the list of supported sensors.VL531Xthat inherits fromI2CSlave.VL531Xclass to be compatible with the current I2C API.VL531Xclass to use slave methods such asreadandwriteinstead of primitive I2C methods.