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 C# Compiler" #1785

Merged
merged 386 commits into from Sep 15, 2022
Merged

"Arduino C# Compiler" #1785

merged 386 commits into from Sep 15, 2022

Conversation

pgrawehr
Copy link
Contributor

@pgrawehr pgrawehr commented Jan 28, 2022

Draft PR, so that I get a regular CI run

This "compiler" (actually a metadata converter and code analyzer) can take a compiled Iot program written using the Arduino binding and flash it as a standalone application to an ESP32 or similar 32-bit microcontroller.

Test and debug your application on the PC using the Arduino binding, and when everything works as expected, just flash it to the ESP32 and run it without the PC.

There are still a lot of open ends, but several more-or-less complex programs already work (see the WeatherStation example). Further documentation (both on usage as well as on internals) is still to be written.

Microsoft Reviewers: Open in CodeFlow

Size is not significantly reduced, impact for functionality is considerable, should rather exclude some specific cases
For now suppressing IConvertible, which bloats up String for no real value.
On the real CPU, memory limits kick in
But need to do the strings as well, or I'll need to
introduce special handling which gets obsolete again afterwards
Currently excluding the startup sequence
Allows running the clock sample on the real hardware. Should not be left like this, though
# Conflicts:
#	src/devices/Arduino/Arduino.csproj
#	src/devices/Arduino/ArduinoAnalogController.cs
#	src/devices/Arduino/ArduinoAnalogInputPin.cs
#	src/devices/Arduino/ArduinoBoard.cs
#	src/devices/Arduino/ArduinoGpioControllerDriver.cs
#	src/devices/Arduino/ArduinoI2cDevice.cs
#	src/devices/Arduino/ArduinoPwmChannel.cs
#	src/devices/Arduino/ArduinoSpiDevice.cs
#	src/devices/Arduino/ConfigurableFirmata/ConfigurableFirmata.ino
#	src/devices/Arduino/FirmataCommand.cs
#	src/devices/Arduino/FirmataDevice.cs
#	src/devices/Arduino/FirmataSpiCommand.cs
#	src/devices/Arduino/FirmataSysexCommand.cs
#	src/devices/Arduino/README.md
#	src/devices/Arduino/SupportedMode.cs
#	src/devices/Arduino/SupportedPinConfiguration.cs
#	src/devices/Arduino/samples/Arduino.Monitor.cs
#	src/devices/Arduino/samples/Arduino.Monitor.csproj
#	src/devices/Arduino/samples/Arduino.sample.cs
#	src/devices/Arduino/samples/Arduino.sample.csproj
#	src/devices/Arduino/samples/CharacterDisplay.cs
#	src/devices/Arduino/tests/Arduino.Tests.csproj
#	src/devices/Arduino/tests/FirmataTestFixture.cs
#	src/devices/Common/CommonHelpers.csproj
#	src/devices/Common/System/Device/Analog/AnalogController.cs
#	src/devices/Common/System/Device/Analog/AnalogInputPin.cs
#	src/devices/Common/System/Device/Analog/TriggerReason.cs
#	src/devices/Common/System/Device/Analog/ValueChangedEventArgs.cs
#	src/devices/Common/System/Device/Analog/ValueChangedEventHandler.cs
#	src/devices/HardwareMonitor/OpenHardwareMonitor.cs
This list will be used to look up relations between complex types,
i.e. Nullable<T> as part of a collection, or types with multiple
generic arguments (i.e. Dictionary<TKey,TValue>)
Required for some Reflection operations on complex
types (such as Dictionary)
Provides a temporary workaround for the missing GC.
Required for GC, because we don't have type
info for unboxed complex value types
…tMerged2

# Conflicts:
#	src/devices/Arduino/samples/Arduino.sample.cs
#	src/devices/Arduino/tests/FirmataTestFixture.cs
#	src/devices/CharacterLcd/Hd44780.cs
Will allow to move methods to a different class later
@pgrawehr
Copy link
Contributor Author

/azp run dotnet.iot

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

# Conflicts:
#	src/devices/Arduino/ArduinoBoard.cs
#	src/devices/Arduino/ArduinoGpioControllerDriver.cs
#	src/devices/Arduino/Encoder7Bit.cs
#	src/devices/Arduino/ExtendedCommandHandler.cs
#	src/devices/Arduino/FirmataDevice.cs
#	src/devices/Arduino/ReconnectingNetworkStream.cs
Need to look into this
@pgrawehr pgrawehr removed * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) help wanted Extra attention is needed labels May 27, 2022
@pgrawehr pgrawehr requested a review from joperezr May 29, 2022 17:03
@pgrawehr
Copy link
Contributor Author

@joperezr Anything open here? I would be happy if this could be merged before I leave for my holidays.

@joperezr
Copy link
Member

Haven't had the chance to take another look. I will take a look at it before Thursday.

@joperezr
Copy link
Member

joperezr commented Jun 1, 2022

Let's revert all of the changes that are going into the azure-pipelines.yml as I would prefer not to block CI due to a problem with this since this is a tool, and the script is pulling an external repo so that seems very fragile. Apart from the fact that our current CI is running in around 10-15 mins, and your two new legs are averaging 60 mins so we definitely don't want to quadruple the build time for PRs for something that won't really add any protection to the shipping code.

Also, seems like part of your changes are doing additional things which we don't want, for example you are changing the vmImage for some of the pools, and you are passing in /macCpuCount to the build command causing a regression in build performance.

@@ -0,0 +1,66 @@
echo on
Copy link
Member

Choose a reason for hiding this comment

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

Per my previous comment, let's just remove this file for now. We can later decide to add a separate pipeline that we can conditionally invoke in some PRs that where it is relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the build script, not the yml file. We might adapt or change this, but deleting won't help.

@@ -0,0 +1,32 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Member

Choose a reason for hiding this comment

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

I think for now we should remove this project (as well as all of the projects underneath this like the samples, Frontend, and tests) from building when build.cmd is invoked using some Exclusions in build.proj file in the root of the repo. 99.9% of the people building the repo won't really care about building them, and this is currently being built since we have:

iot/build.proj

Line 38 in 6be64b7

<_BuildToolProjects Include="$(MSBuildThisFileDirectory)tools\**\*.csproj" />

Alternatively we can disable BuildTools target entirely on the default build.cmd and only enable it when people care about it, so perhaps in your Readme you can add a line saying that they should call build like build.cmd /p:ToolsBuild=true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I basically agree, but is it worth the extra complexity? The tools take like 2 seconds of the overall build time.

@pgrawehr
Copy link
Contributor Author

Let's revert all of the changes that are going into the azure-pipelines.yml as I would prefer not to block CI due to a problem with this since this is a tool, and the script is pulling an external repo so that seems very fragile.

Actually, it also tests the existing Arduino binding against (simulated) hardware. What about pinning the revision on the external repo, so that changes there would only affect this when explicitly updated?

Apart from the fact that our current CI is running in around 10-15 mins, and your two new legs are averaging 60 mins so we definitely don't want to quadruple the build time for PRs for something that won't really add any protection to the shipping code.

Yes, that's indeed bad. I'll (try to) add a variable to normally not run the tool tests.

Also, seems like part of your changes are doing additional things which we don't want, for example you are changing the vmImage for some of the pools, and you are passing in /macCpuCount to the build command causing a regression in build performance.

Sure, removed those changes again, didn't intend to leave that in.

I'll look into the "nits" later. Those should be easy.

@@ -89,6 +90,28 @@ stages:
artifact: config
condition: eq(variables['BuildConfiguration'], 'Release')

- job: Windows_ArduinoIntegration
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add this to the main build yet. We should either make a new pipeline for this that can be invoked manually for a PR (probably ideal) or have some sort of logic that detects for changes on the Arduino or compiler bindings and would only run this job if any changes are detected. This latter option is less ideal, mainly cause you can't really invoke it if you want to run it even when changes are not detected, and it is also less maintainable since could be prone to breaking if files get moved around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually don't see much of a problem the way it is now. This is easily identifyable as it is a separate build, and when it breaks on a PR it can quickly be checked whether the breakage is due to a bad change in the Arduino binding or whether it is due to some problem completely unrelated to the PR. This is much like we do with the hardware tests now. The number of PRs we have is not so big that this extra check cost really a lot of time right now.

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Left one comment about removing this from the main pipeline, but other than that, this looks good to go.

@pgrawehr pgrawehr merged commit 55a4ec0 into dotnet:main Sep 15, 2022
@pgrawehr pgrawehr deleted the ArduinoCsPr branch September 15, 2022 11:46
@pgrawehr
Copy link
Contributor Author

Merged as-is. Instead of endless discussions on possible build issues I think it's easier to address those if they show up. It would be easy to disable the Arduino build if it kept breaking the build unjustified.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-tools Tools for testing while developing for .NET Core IoT enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants