Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Move classes and rename to be consistent throughout API #126

Closed
3 of 4 tasks
shaggygi opened this issue Dec 29, 2018 · 16 comments
Closed
3 of 4 tasks

[Proposal] Move classes and rename to be consistent throughout API #126

shaggygi opened this issue Dec 29, 2018 · 16 comments
Labels
area-System.Device.Gpio Contains types for using general-purpose I/O (GPIO) pins Design Discussion Ongoing discussion about design without consensus documentation Documentation bug or enhancement, does not impact product or test code

Comments

@shaggygi
Copy link
Contributor

shaggygi commented Dec 29, 2018

This proposal is a collection of different modifications and grouped here as there are many breaking changes being proposed. The intent is to review, decide and update (if needed) namespaces, class names and file locations in a consistent structure between the different types of interfaces within System.Device.Gpio API.

This seems to fit with @terrajobst comments during this design review, as well.

Rationale and Usage

Since System.Device.Gpio is in early previews, it seems like the opportune time to review and decide how to structure the core components. This will reduce breaking changes in the future.

Examples

  • GPIO and PWM have abstract classes based on Drivers and are located under the *.Drivers folder. This includes RaspberryPi3Driver, UnixDriver.Linux.cs, etc.. Gpio is not in the file names where I2C/Pwm/SPI are included in the file names.
  • I2C and SPI are based on Devices. However, there are implementations under the *.Drivers folders.

Proposed Changes

The proposed changes are to rename and move files as shown in the folder structure below. To simplify, this attempts to point out only the changes for the files being modified.

GPIO Folder

  • TODO
  1. Drivers and related classes renamed to include "Gpio".
  2. GpioDriver.cs moved to Drivers folder.
Device
  Gpio
    Drivers
      GpioDriver.cs
      HummingBoardGpioDriver.cs
      HummingBoardGpioDriver.Linux.cs
      HummingBoardGpioDriver.Windows.cs
      RaspberryPi3GpioDriver.cs
      RaspberryPi3GpioDriver.Linux.cs
      RaspberryPi3GpioDriver.Windows.cs
      UnixGpioDriver.Linux.cs
      UnixGpioDriver.Windows.cs
      UnixGpioDriverPin.Linux.cs  // Also simplified by removing Device
      Windows10GpioDriver.Linux.cs
      Windows10GpioDriver.Windows.cs
      Windows10GpioDriverPin.Windows.cs
    GpioPinChangeEventHandler.cs
    GpioPinEventTypes.cs
    GpioPinMode.cs
    GpioPinNumberingScheme.cs
    GpioPinValue.cs
    GpioPinValueChangedEventArgs.cs

I2C Folder

  1. Folder renamed from "Drivers" to "Devices".
  2. Namespaces be renamed from "System.Device.I2c.Drivers" to "System.Device.I2c"
  3. I2cDevice.cs moved to Devices folder.

See #118 and #534 for related details

Device
  I2c
    Devices
      I2cDevice.cs
      I2cDevice.Linux.cs
      I2cDevice.Windows.cs
      [other devices here that were not moved or renamed]
    I2cConnectionSettings.cs (didn't change; showing for location purposes)

PWM Folder

  1. PwmDriver.cs moved to Drivers folder.

See #204 for related details

Device
  Pwm
    Drivers
      PwmDriver.cs

SPI Folder

  1. Folder renamed from "Drivers" to "Devices".
  2. Namespaces be renamed from "System.Device.Spi.Drivers" to "System.Device.Spi"
  3. SpiDevice.cs moved to Devices folder.

See #119 and #534 for related details

Device
  Spi
    Devices
      SpiDevice.cs
      SpiDevice.Linux.cs
      SpiDevice.Windows.cs
      [other devices here that were not moved or renamed]
    SpiConnectionSettings.cs (didn't change; showing for location purposes)

Open Questions

  1. It seems a little redundant to have the interface in the file name. For example, GpioPinMode. Is this too redundant knowing it is under the Gpio folder/namespace? It seems like differentiating would be okay, especially when mixing different pin types in code files (e.g. Bindings with a mixture of digital, analog and PWM pins). This would help when adding "usings" or typing entire namespace out.
@shaggygi shaggygi changed the title Should GPIO drivers be renamed to include Gpio [Proposal] Classes should be moved and renamed to be consistent throughout API Jan 4, 2019
@shaggygi shaggygi changed the title [Proposal] Classes should be moved and renamed to be consistent throughout API [Proposal] Move classes and rename to be consistent throughout API Jan 4, 2019
@shaggygi
Copy link
Contributor Author

shaggygi commented Feb 1, 2019

@joshfree @joperezr Just wanted to ping this one again now that Preview 2 is out.

@joperezr joperezr added Design Discussion Ongoing discussion about design without consensus area-System.Device.Gpio Contains types for using general-purpose I/O (GPIO) pins labels Feb 12, 2019
@joperezr
Copy link
Member

Hey @shaggygi thanks for bringing up all of these good questions. I do agree that there are some inconsistencies across our library today, which should get fixed before we ship a stable package. For example, I don't like the fact that Gpio and PWM have a controller which is what you interact with and don't have a specific device object (which in this case would be a GpioPin or PwmPin) but in I2c and Spi we do have the individual devices and don't have a controller for the whole bus. I also think we are lacking some behavior interfaces which should be added which those are tracked by other issues.

In general for your other questions, I don't think that adding Gpio into all of the classnames under Gpio makes sense as since you point out, it does add redundancy when you see the fully qualified name. That said, we do have some counter-examples for that. Let's keep this issue around and use it to discuss concrete examples that should change for the next previews so that we can make things more consistent.

@joperezr joperezr added this to the 3.0 milestone Feb 12, 2019
@shaggygi
Copy link
Contributor Author

Sounds good. It might be worth to keep some action items (checkboxes) on this as the main place to confirm the overall API changes that need to happen to call it complete (even if there are other issues created for specific action items).

I agree the namespaces help separate out (although preferred to include). I just want to make sure (if possible) to make different sections close that make sense. As noted... I'm okay with System.Device.Gpio.UnixDriver, but we should also have System.Device.Pwm.UnixDriver.

And while using the namespace to separate will help, I'm more worried when we begin working on future interfaces and implement bindings that have multiple functions. For example, now that the IGpioController is pushed, I plan to begin working on a new binding that provides GPIO and PWM. The plan is to later add what is pushed for IPwmController (or related solution). By not having the function in name, it might get a little confusing like the UnixDriver example as we could be using both.

Thanks again for managing this... it's a booger 😄

@joperezr joperezr added the documentation Documentation bug or enhancement, does not impact product or test code label Apr 17, 2019
@shaggygi
Copy link
Contributor Author

@joperezr we might need to get focused on this one soon. Thoughts?

@joperezr
Copy link
Member

joperezr commented May 28, 2019

Agreed, all of the issues that are flagged today as 3.0 are things we have to finish in the next few weeks, so we will definitely get to this one in the next couple of weeks.

@shaggygi
Copy link
Contributor Author

shaggygi commented Jun 16, 2019

@joperezr I updated the I2C and SPI parts in initial proposal above. Hope to have more details on #118 and #119 soon. And even if those proposals don't get completed by v1 (which I hope), the related changes in this proposal could be updated with or without and won't affect them.

@joperezr
Copy link
Member

I saw the comment on the I2c proposal about this. I think having that change is fine.

@shaggygi
Copy link
Contributor Author

@joperezr @krwq Now that I2C and SPI is winding down, would it be possible to go ahead and put in a PR for updating the related Drivers > Devices folders and namespaces? I could probably complete those 2 parts of this issue over the weekend.

Related to those, the only thing I had a question on was moving the xxxDevice.cs to the folder. I like those being grouped with the other ...Device.cs files, but on the fence keeping as is. And if they are moved in Devices folder, should the namespace be updated for xxxDevice.cs, as well?

@krwq
Copy link
Member

krwq commented Jun 26, 2019

@shaggygi do you mean specifically those 3:

  • Folder renamed from "Drivers" to "Devices"
  • Namespaces be renamed from "System.Device.I2c.Drivers" to "System.Device.I2c.Devices" (and similar for other similar stuff)
  • I2cDevice.cs moved to Devices folder

@joperezr are you ok with that? It looks fine with me

regardless - could you make sure to do any file renames (+ csproj adjustments) in a separate commit to make it easier to review?

@shaggygi
Copy link
Contributor Author

@krwq yup, those are the 3 things to focus on for both. I can do separate PRs for each type if that helps break down since this is breaking changes.

@krwq
Copy link
Member

krwq commented Jun 26, 2019

@shaggygi assuming @joperezr agrees with this work I think ideally I'd at minimum do 2 PRs or more depending on the diff size:

  • all renames, no namespace changes, don't touch .cs files, only adjust paths in csproj - otherwise the diff might break (fine to have a mismatch between file name and class name as an intermediate step)
  • rename namespaces/classes one by one (make sure to update all READMEs and things like that), you can add more to fill up to say 500 lines of code diff
  • later let's list all the changes and search on major search engines for the names and see if we should announce it more directly on various forums/threads in case we already got some adoption with current names

cc: @richlander - TL;DR; we're thinking of doing some renaming for the sake of consistency - this matches with previous API review feedback

@joperezr
Copy link
Member

@shaggygi those three changes that @krwq mentioned above are approved.

@joperezr
Copy link
Member

joperezr commented Jul 9, 2019

I suppose that the PWM PR that we have open is the last thing about this issue that we want to fix? @shaggygi @krwq

@shaggygi
Copy link
Contributor Author

shaggygi commented Jul 10, 2019

@joperezr Still need to move the I2c/Spi Device classes back to their root (based on PWM discussion), but need to finalize #569 before I can update.

I think GPIO still needs a quick review. For example...

  • UnixDriverDevicePin.Linux.cs could have Device removed from name.
  • LibgpiodDriverEventHandler.Linux.cs might could move to Drivers folder.
  • Still think some files/classes should have a prefix of Gpio, but I wave the white flag at this point. 😄

@joperezr
Copy link
Member

So for the ones you mentioned, I2c and Spi classes moving won't need to happen before 3.0, because this only involves the specific platform devices, which are internal, so no problem if we don't do that for now.
Same goes for the other cases, UnixDriverDevicePin and LibgpiodDriverEventHandler are internal, so no problem if we want to rename/move them later.

Because of that, I will move this issue to Future for our internal tracking, but feel free to push back if you think otherwise.

@joperezr joperezr modified the milestones: 3.0, Future Jul 10, 2019
@krwq
Copy link
Member

krwq commented Jan 7, 2021

[Triage] I think the main points are already fixed and we do not have enough motivation to continue this effort so closing this issue as fixed.

@krwq krwq closed this as completed Jan 7, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Device.Gpio Contains types for using general-purpose I/O (GPIO) pins Design Discussion Ongoing discussion about design without consensus documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

No branches or pull requests

3 participants