Conversation
|
Could you please split the PRs by differentiating the tasks? I see changes to the lang versions along with other stuff not related to the binding. |
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new device binding for the INA236 voltage and current sensor, along with significant improvements to the repository's testing infrastructure and language version standardization.
Key Changes:
- New INA236 device binding with complete implementation including tests, samples, and documentation
- Introduction of I2cSimulatedDeviceBase class to provide a reusable testing infrastructure for I2C devices
- Standardization of C# language version to 12 across the entire repository
- Refactoring of existing Tca955x tests to use the new simulated device infrastructure
Reviewed changes
Copilot reviewed 79 out of 79 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| src/devices/Ina236/Ina236.cs | Main INA236 device implementation with voltage/current/power monitoring capabilities |
| src/devices/Ina236/Ina236OperatingMode.cs | Operating mode enumeration for device configuration |
| src/devices/Ina236/Ina236Register.cs | Register address definitions |
| src/devices/Ina236/README.md | Device documentation and usage examples |
| src/devices/Ina236/samples/Program.cs | Sample code demonstrating device usage |
| src/devices/Ina236/tests/Ina236Tests.cs | Unit tests for INA236 device |
| src/devices/Ina236/tests/SimulatedIna236.cs | Simulated device for testing |
| src/devices/Common/System/Device/I2c/I2cSimulatedDeviceBase.cs | New base class for simulating I2C devices in tests |
| src/devices/Tca955x/tests/Tca955xSimulatedDevice.cs | New simulated device using the base infrastructure |
| src/devices/Tca955x/tests/Tca9554Tests.cs | Refactored tests using new infrastructure |
| src/devices/Tca955x/tests/Tca9555Tests.cs | Refactored tests using new infrastructure |
| Directory.Build.props | Updated default LangVersion to 12 |
| samples/Directory.Build.props | Updated default LangVersion to 12 |
| Multiple .csproj files | Removed explicit LangVersion settings to use default |
| src/devices/Gpio/README.md | Fixed table formatting and typos |
| src/devices/Tca955x/README.md | Fixed spelling errors |
| samples/README.md | Updated .NET version requirements |
dotnet-policy-service
bot
added
the
area-device-bindings
Ellerbach
left a comment
Ellerbach
left a comment
There was a problem hiding this comment.
couple of comments, looks nice!
src/devices/Ina236/Ina236.cs
Outdated
| /// Type B has addresses 0x44 to 0x47, depending on the ADDR pin. | ||
| /// </summary> | ||
| public const int DefaultI2cAddress = 0x40; | ||
| private I2cDevice _i2cDevice; |
There was a problem hiding this comment.
Changed it to nullable, so I can throw deterministic ObjectDisposedException's.
Generic implementation is difficult, one problem is the different argument sizes and the fact that the endianess on the bus may change.
Will make it easier in future.
Ellerbach
left a comment
Ellerbach
left a comment
There was a problem hiding this comment.
Overall looks good. Check the copilot comments, those are about nits (variables names, xml comments, things like this) and all valid. Approving already as nothing blocking. And nice refactoring for the I2C mock device btw :-)
| using Iot.Device.Arduino; | ||
| using UnitsNet; | ||
|
|
||
| using ArduinoBoard board = new ArduinoBoard("COM5", 115200); |
There was a problem hiding this comment.
Can you give an example that is using classic RPI? So, it makes it easier for people to use. Or at least put the code in comments the initialization and the pins so that everyone can choose to use a RPI instead?
Add a new Binding for INA236 voltage and current sensor. It's similar to the existing binding INA219 in functionality, but the registers are quite different, so that a common base class is not useful. Could think of a common interface, though.
Alarm features are not implemented as the breakouts from most suppliers don't seem to have the ALRT pin wired out.
Microsoft Reviewers: Open in CodeFlow