Rename i2c structure again#543
Rename i2c structure again#543krwq merged 15 commits intodotnet:masterfrom shaggygi:rename-i2c-structure-again
Conversation
|
what you have overall looks good. I remember there are couple of XML docs which mention WIndows/LinuxI2c/Spi which we should fix since they are internal now |
| using System; | ||
| using System.Device.Gpio; | ||
| using System.Device.I2c; | ||
| using System.Device.I2c.Devices; |
There was a problem hiding this comment.
looks much nicer now, two namespaces were kinda confusing
Good point. Working on it now. |
|
@krwq I think this finishes up here. I'll have a PR for SPI soon. |
krwq
left a comment
krwq
left a comment
There was a problem hiding this comment.
Except for >> in the xml docs LGTM
|
@krwq ok. one more time 😄 I found a few others along the way. |
|
@shaggygi we will need to fix the build as part of this PR so if needed you'll need to do SPI as well |
The build is not failing because of SPI. It is failing because the bindings package references the live build version of GPIO, I will point out which changes are required in order to fix the CI Build |
| /// Represents an I2C communication channel running on Unix. | ||
| /// </summary> | ||
| public class UnixI2cDevice : I2cDevice | ||
| internal class UnixI2cDevice : I2cDevice |
There was a problem hiding this comment.
We don't need an internal class here. Instead what we need is simply to move all of this code under I2CDevice.Linux.cs. Sorry if that didn't make sense before. Ideally what we want is not to need to have extra internal classes to shell out to, instead we just want to have one I2CDevice class, which in Linux has the copied implementation from the previous UnixI2CDevice, and in windows it uses what used to be Windows10I2cDevice. Does that make sense?
There was a problem hiding this comment.
Sure. Lemme review. Man, y'all are making sweat here 🏃 😓
There was a problem hiding this comment.
@shaggygi don't do anything yet, we're talking offline about this right now
There was a problem hiding this comment.
Hey @shaggygi sorry for randomizing you, So I think we want to talk a bit more about the comment I gave and maybe have a small API review for it. In the meantime, I'm fine with taking this as is, except that we should fix the CI Build, I'll let you know how to do that.
There was a problem hiding this comment.
@joperezr no prob. i was expecting some tap dancing with this breaking change 😄 i'll check back in a few.
|
What you will want to change in order to fix the BuildPackages build, you will need to change these lines: iot/src/Iot.Device.Bindings/Iot.Device.Bindings.csproj Lines 21 to 26 in 80b8fdd to instead look like: <ItemGroup>
<PackageReference Include="System.Drawing.Common" Version="4.6.0-preview4.19164.7" />
<PackageReference Include="System.Device.Gpio" Version="$(SystemDeviceGpioPackageVersion)" />
</ItemGroup>As for the other build breaks that you started getting, seems like you want to remove readonly from some of the changes that you updated for the Dispose method. In fact, I'd be fine if you want to just remove e941613 entirely from your PR so that we only deal with the namespace change here. |
joperezr
left a comment
joperezr
left a comment
There was a problem hiding this comment.
Changes look good to me now. This is ready to go assuming CI is green. Thanks a lot for fixing this @shaggygi and sorry for the confusion, I missed the part of the issue where the Create method was agreed upon and the other devices were supposed to be made internal.
|
@joperezr No problem and was actually fun. If CI is green the SPI PR is a few seconds away 👍 |
3 participants
This addresses the I2C portion of #534