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

Arduino/Firmata device support #1039

Merged
merged 56 commits into from
Feb 25, 2021
Merged

Arduino/Firmata device support #1039

merged 56 commits into from
Feb 25, 2021

Conversation

pgrawehr
Copy link
Contributor

@pgrawehr pgrawehr commented Apr 6, 2020

Fixes #262

This adds support for controlling an Arduino board directly from a PC. The arduino board will run a special firmware called "Firmata", that interprets the commands from the PC and set it to hardware. See Readme for details.

This also includeds an ArduinoBoard class, which is a prototype for #878. I'll later push more commits that will show how that would look like with the existing Windows/Linux drivers (but that's somehow also dependent on #1038, because things get complicated otherwise, see #1037 for one of the issues)

@Ellerbach Can you check whether that interface would fit your FT4222 as well?

The first implementation for the firmata driver was from https://github.com/ms-iot/remote-wiring. MIT license.

@joperezr
Copy link
Member

NIT: I believe Arduino is kind of a very Generic Name to have this be the Arduino.csproj. Might get confusing people thinking that this is a full Arduino binding which is not really the case. Can we instead name it as Arduino.Firmata or something like that?

@joperezr
Copy link
Member

I’ll continue reviewing this weekend but since you wanted to know I definitely think we are on the right track here. I love how the API surface is minimal and very easy to use, and won’t require new knowledge as in the end you are really just exposing spi, i2c and gpio devices.

@pgrawehr
Copy link
Contributor Author

NIT: I believe Arduino is kind of a very Generic Name to have this be the Arduino.csproj. Might get confusing people thinking that this is a full Arduino binding which is not really the case. Can we instead name it as Arduino.Firmata or something like that?

Maybe use "Firmata" as namespace and ArduinoXyz for the concrete class names?

@Ellerbach
Copy link
Member

Can you check whether that interface would fit your FT4222 as well?

@pgrawehr, it's a very complementary approach. Same philosophy: give something easy on a "normal" Windows or Mac or Linux an access to GPIO, SPI and I2C with the PWM flavor and more.

You went for a similar approach with a specific core communication element (your ArduinoBoard and the specific Firma classes). And for specific GPIO, I2C and PWM, for the driver which is the approach I took as well for the FT4222. I'm just surprised that there is no SPI support in Firma. Is there a way to get it somehow? That would be super nice as well.

Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All up, same comment at @joperezr, I really like the project, the fact that like for the FT4222 we have simple drivers and something very easy to use. That gives more way to enjoy .NET Core and IoT on various hardware! Even if the cost of performance can be a blocker for some of the devices. So would be good to explain in the Readme some of the limitations.

src/devices/Arduino/ArduinoAnalogControllerDriver.cs Outdated Show resolved Hide resolved
src/devices/Arduino/CommonFirmataFeatures.ino Outdated Show resolved Hide resolved
src/devices/Arduino/ArduinoBoard.cs Outdated Show resolved Hide resolved
src/devices/Arduino/ArduinoBoard.cs Outdated Show resolved Hide resolved
src/devices/Arduino/ArduinoI2cDevice.cs Show resolved Hide resolved
src/devices/Arduino/FirmataDevice.cs Outdated Show resolved Hide resolved
src/devices/Arduino/SupportedMode.cs Outdated Show resolved Hide resolved
src/devices/Arduino/README.md Show resolved Hide resolved
@krwq
Copy link
Member

krwq commented Sep 16, 2020

@pgrawehr what's the remaining work here aside from the merge conflicts?

@pgrawehr
Copy link
Contributor Author

@pgrawehr what's the remaining work here aside from the merge conflicts?

I don't remember in detail, but one of the open points is how we go further with the board PR. This basically bases on that one (or actually, a slightly older proposal of it). So it needs to be clarified whether the concept matches our future strategy.

@krwq
Copy link
Member

krwq commented Oct 7, 2020

@pgrawehr what specific pieces of board abstraction is must have for this PR to move forward?

@pgrawehr
Copy link
Contributor Author

pgrawehr commented Oct 8, 2020

@krwq : it's been a while now, so I'm not 100% sure and will need to check again.
What's certainly needed is the unsealing of the GpioController, ideally also the abstract board class, but that could likely also be put in later (eventually with breaking changes though, when different method names will be used)

@Ellerbach Ellerbach added the area-System.Device.Gpio Contains types for using general-purpose I/O (GPIO) pins label Oct 14, 2020
@joperezr
Copy link
Member

[Triage] We would like to go forward with this PR but it seems like it is not clear which are the blockers here. @pgrawehr when you have some time do you mind checking what you need from us in order to move this forward?

@pgrawehr
Copy link
Contributor Author

Discussed via Teams: I'll need to update the PR with my latest Arduino branch. The PR depends on the GpioController unsealing, so I'll probably copy that change here, so it can be done without the full impact of #1128.
The proposed AnalogController can stay in Iot.Devices for now.

Some breaking changes are expected when any of the following is finally decided on:

  • The board base class is defined
  • AnalogController is moved to System.Devices namespace

@Ellerbach
Copy link
Member

The proposed AnalogController can stay in Iot.Devices for now.

For the ADC we can as well get inspiration an try to align with the nanoFramework one: https://github.com/nanoframework/lib-Windows.Devices.Adc. There are good elements that we already discussed in the related issues #878 and #90.

@Ellerbach
Copy link
Member

AnalogController is moved to System.Devices namespace

And that does remind me as well that there are quite some Analog sensors in GrovePi. Clearly soem real work on that would allow to reuse some of the sensors but as well having GrovePi being a proper GpioController and AnalogController at the same time in a more reusable way.

@pgrawehr
Copy link
Contributor Author

The proposed AnalogController can stay in Iot.Devices for now.

For the ADC we can as well get inspiration an try to align with the nanoFramework one: nanoframework/lib-Windows.Devices.Adc. There are good elements that we already discussed in the related issues #878 and #90.

Not sure I can fullfull all of those requirements just yet, but I'll try. The Nanoframwork API is conceptually a bit different than the approach here, but it is also pretty small.

Comment on lines 225 to 228
if (!_initialized)
{
Initialize();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment above, consider moving this logic into Initialize (currently it's there inside of the lock, but consider doing this check before and inside of the lock)

src/devices/Arduino/README.md Outdated Show resolved Hide resolved
src/devices/Arduino/README.md Outdated Show resolved Hide resolved
src/devices/Arduino/README.md Outdated Show resolved Hide resolved
src/devices/Arduino/README.md Outdated Show resolved Hide resolved
src/devices/Arduino/README.md Outdated Show resolved Hide resolved
@krwq
Copy link
Member

krwq commented Feb 22, 2021

@pgrawehr looks good to me with some comments

src/devices/Arduino/README.md Outdated Show resolved Hide resolved
@krwq
Copy link
Member

krwq commented Feb 25, 2021

@pgrawehr I think #1039 (review) got missed other than that LGTM

Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend to fix the missed comment but nothing blocking to merge this from my perspective. Thanks for this great contribution!

@krwq krwq merged commit 22d420c into dotnet:main Feb 25, 2021
@krwq
Copy link
Member

krwq commented Feb 25, 2021

:shipit:

@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arduino/Firmata support
4 participants