Add a driver for the RevColorSensor v2 - #9380
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cea25d8271
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| private byte[] readRegister(Register register, int count) { | ||
| byte[] data = new byte[count]; | ||
| if (requireOpen().read(register.m_address | COMMAND_BIT, count, data)) { |
There was a problem hiding this comment.
On a real TMD3782, command bits 6:5 equal to 00 select the repeated-byte protocol, so the 11-byte STATUS read sends 0x80 | 0x13 and returns the status register repeatedly instead of advancing through the color and proximity registers. Consequently update() decodes bogus values (for example, status 0x03 produces 0x0303 for every channel) even though the simulator tests pass. Set the auto-increment command bit for multi-byte reads, and make the corresponding change in the C++ ReadRegister() implementation, which currently uses the same command.
Useful? React with 👍 / 👎.
| int control = | ||
| (data[0] & 0xFF & ~(GAIN_MASK | LED_DRIVE_MASK)) | ||
| | CONTROL_IR_DIODE | ||
| | m_gain.m_value | ||
| | m_ledDrive.m_value; |
There was a problem hiding this comment.
If the sensor's CONTROL register retains a prior configuration, this mask preserves both PDIODE bit 4 and the proximity-gain bits 3:2 even though they directly affect proximity readings. OR-ing CONTROL_IR_DIODE into a retained bit 4 can select both photodiodes instead of the calibrated IR channel, while a retained nondefault proximity gain changes every raw value and invalidates the default distance calibration. Disabling the sensor does not reset these fields, so configuration should explicitly clear/set the complete diode and proximity-gain fields; the mirrored C++ expression has the same problem.
Useful? React with 👍 / 👎.
| int cycles = static_cast<int>( | ||
| std::ceil(milliseconds / INTEGRATION_CYCLE_MILLISECONDS)); |
There was a problem hiding this comment.
For a positive finite duration larger than roughly INT_MAX * 2.4 ms, the documented saturation path instead converts an out-of-range floating-point value directly to int, which is undefined behavior in C++. Such a value can therefore corrupt the ATIME calculation rather than clamp to the advertised 614.4 ms maximum. Clamp the floating-point cycle count to 256 before performing the integer conversion.
Useful? React with 👍 / 👎.
| * reports as not yet valid are reported through DeviceStatus instead of | ||
| * throwing an exception, and the previously cached measurements are preserved. | ||
| */ | ||
| class RevColorSensorV2 { |
There was a problem hiding this comment.
A repository-wide search of drivers/src/main/python and the generated binding files finds no RevColorSensorV2 semiwrap definition, header registration, package export, or Python test. Unlike the other public native drivers in this module, this class is therefore omitted from the wpilib-drivers Python wheel, so Python teams cannot use the new sensor support at all. Add the semiwrap and package entries and regenerate the RobotPy binding files.
Useful? React with 👍 / 👎.
|
I think it could be argued org.wpilib.drivers.color would be more accurate, but otherwise yeah this is the right place. Do you also intend to port the v3 version? |
|
Isn't the REV Color Sensor V3 still supported in REVLib? |
|
I believe it is, but, similarly to the A301 and the Pinpoint, I think it's a common enough thing (at least for FTC teams) to warrant not needing to add the vendordep. |
3 participants
While Rev no longer sells the v2 of the RevColorSensor, many teams have a bunch of these because they were included in the electronics kit when you bought it directly from FIRST for many years.
Before going further, I wanted to se if this is the right idea and the right place.
(Yes, this was Claude generated by pointing it at the source in the FTC SDK and the one sample that was here.)