Adding RPI dtoverlay to check in configuration file #2058

Ellerbach · 2023-03-20T16:40:02Z

Adding RPI dtoverlay to check in configuration file. This is complementary from the existing elements that will return the default pins and configurations.
Those extra functions will actually check that the /boot/config.txt file contains the proper activation and will return the actual pin setup based o this.

Microsoft Reviewers: Open in CodeFlow

joperezr · 2023-03-20T20:08:38Z

cc @pgrawehr

This is one of the changes that might benefit from running the Arduino pipeline on demand. We should probably wait before merging this (or adding those changes here as well) in order to ensure that these changes aren't breaking the Arduino Board and compiler build.

pgrawehr · 2023-03-20T20:19:18Z

Basically a nice idea, but I have some reservations regarding stability. Text comparisons aren't really stable for things like this, and we wouldn't even know whether the configuration file is the one that was used during boot.

What problem are you trying to solve?

Ellerbach · 2023-03-21T09:24:15Z

What problem are you trying to solve?

Main problem we're trying to solve is find a way to know in our test if a specific function is activated on the hardware. We know that one of the RPI do not have PWM but we don't know which one. And with a bit of a hack, still in the tests, we can add the potential missing string and reboot the board so next time the function will be activated. And if you think about it, it may help so we don't really need anyone to physically connect to the machines, adjust the config file.

The other element is for people to know if a specific function is activated or not. And we may even build like what we want to do for the test the string they have to add in the config file.

Text comparisons aren't really stable

In general, I agree, in this case, if you don't have those exact strings, it won't get activated.

we wouldn't even know whether the configuration file is the one that was used during boot

On RPI, it's always the same by default. And for people who'll really have to use this, the chances they'll change the file is very very low! Now, as you've seen, you can as well specify a path to any file, so if you've changed it and still want to check, then you can. That's what I am using for the tests.

We can as well add serial and few others btw.

src/devices/Board/RaspberryPiBoard.cs

src/devices/Board/samples/Board.Sample.csproj

krwq · 2023-03-21T11:47:20Z

src/devices/Board/tests/RpiBoardDtOverlayTests.cs

+        [Fact]
+        public void CheckI2cConfig()
+        {
+            board.ConfigurationFile = Path.Combine("ConfigFiles", "config.I2c.txt");


do we also want to add actual tests for System.Device.Gpio.Tests? You could do similar test but use default path (make sure to add [Trait(...)] attribute similar to what there already is for PWM/SPI/I2C tests

I added a specific test that output the configuration. It will always pass, it's not intended to fail, just outputting the configuration. I guess it's a good compromise

We can once merged find the best way to use it in the other tests

krwq

Overall looks good, it would be good to use these in System.Device.Gpio.Tests - that would simplify diagnostics of CI failures

Ellerbach · 2023-03-21T15:44:29Z

Overall looks good, it would be good to use these in System.Device.Gpio.Tests - that would simplify diagnostics of CI failures

I think I will have to write specific tests to see what's activated because it's a bit of a chicken and egg then. If System.Device.Gpio.Tests depends on Board which depends back on System.Device.Gpio, not sure how that can work ;-)

So I was more think of a simple test just outputting the results of the I2C activation, SPI, PWM. So we know if it's there or not. And if there, the result for the pins setup as well. Makes sense?

pgrawehr · 2023-03-22T12:50:53Z

What problem are you trying to solve?

Main problem we're trying to solve is find a way to know in our test if a specific function is activated on the hardware. We know that one of the RPI do not have PWM but we don't know which one. And with a bit of a hack, still in the tests, we can add the potential missing string and reboot the board so next time the function will be activated. And if you think about it, it may help so we don't really need anyone to physically connect to the machines, adjust the config file.

The other element is for people to know if a specific function is activated or not. And we may even build like what we want to do for the test the string they have to add in the config file.

Yea, but wouldn't we just fail anyway, because the corresponding /dev/ node (e.g. /dev/pwm-0) is not there? Maybe we should (additionally?) make sure that we get a nice looking error message in such a case.

Text comparisons aren't really stable

In general, I agree, in this case, if you don't have those exact strings, it won't get activated.

What about spaces and other white space in the strings? Wouldn't the settings still be valid? Or parameters in different order?

we wouldn't even know whether the configuration file is the one that was used during boot

On RPI, it's always the same by default. And for people who'll really have to use this, the chances they'll change the file is very very low! Now, as you've seen, you can as well specify a path to any file, so if you've changed it and still want to check, then you can. That's what I am using for the tests.

Yes, it's physically the same file. But it might have been changed without a reboot. So the current state of /boot/config.txt might not match the actual device configuration.

We can as well add serial and few others btw.

Ellerbach · 2023-03-22T13:14:38Z

Yea, but wouldn't we just fail anyway, because the corresponding /dev/ node (e.g. /dev/pwm-0) is not there?

We would. It's just that you can know if things are setup properly or not.

What about spaces and other white space in the strings? Wouldn't the settings still be valid? Or parameters in different order?

Spaces removed from beginning and trailing. They are not allowed in the middle of the string. The setup already take into consideration those. There maybe edge cases and I need to run few physical checks for some of those. The good news is that it's very strict all up.

src/devices/Board/tests/RpiBoardDtOverlayTests.cs

pgrawehr · 2023-03-25T16:58:23Z

src/devices/Board/config.Spi.txt

@@ -0,0 +1,2 @@
+dtparam=spi=on"


There's a config.Spi.txt also in the tests\ConfigFiles folder. I think this one is a duplicate.

src/devices/Board/RaspberryPiBoard.cs

src/devices/Board/tests/ConfigFiles/config.I2c.txt

src/devices/Board/tests/RpiBoardDtOverlayTests.cs

joperezr · 2023-03-30T19:32:53Z

/azp run

azure-pipelines · 2023-03-30T19:33:03Z

Azure Pipelines successfully started running 1 pipeline(s).

joperezr · 2023-04-06T15:48:29Z

Thanks @Ellerbach

Adding RPI dtoverlay check in configuration

3539499

ghost added the area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio label Mar 20, 2023

Ellerbach requested a review from pgrawehr March 20, 2023 16:40

krwq reviewed Mar 21, 2023

View reviewed changes

src/devices/Board/RaspberryPiBoard.cs Outdated Show resolved Hide resolved

krwq reviewed Mar 21, 2023

View reviewed changes

src/devices/Board/samples/Board.Sample.csproj Outdated Show resolved Hide resolved

krwq reviewed Mar 21, 2023

View reviewed changes

krwq approved these changes Mar 21, 2023

View reviewed changes

Ellerbach added 3 commits March 21, 2023 16:14

adjusting based on PR feedback

94073d1

fixing duplicate compile

330139c

adjusting sample csproj

c1ca652

krwq approved these changes Mar 22, 2023

View reviewed changes

Adjust test to use output

b46547e

pgrawehr reviewed Mar 25, 2023

View reviewed changes

src/devices/Board/tests/RpiBoardDtOverlayTests.cs Outdated Show resolved Hide resolved

pgrawehr requested changes Mar 25, 2023

View reviewed changes

ghost added Needs: Author Feedback We are waiting for author to react to feedback (action required) and removed Needs: Author Feedback We are waiting for author to react to feedback (action required) labels Mar 25, 2023

Adjusting based on PR feedback, adding PWM output in S.D.Gpio PWM test

7ff4f15

pgrawehr approved these changes Apr 1, 2023

View reviewed changes

removing nit file

0ac2752

joperezr merged commit 82df033 into dotnet:main Apr 6, 2023
9 checks passed

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding RPI dtoverlay to check in configuration file #2058

Adding RPI dtoverlay to check in configuration file #2058

Ellerbach commented Mar 20, 2023 •

edited by ghost

joperezr commented Mar 20, 2023

pgrawehr commented Mar 20, 2023

Ellerbach commented Mar 21, 2023

krwq Mar 21, 2023

Ellerbach Mar 21, 2023

Ellerbach Mar 21, 2023

krwq left a comment

Ellerbach commented Mar 21, 2023

pgrawehr commented Mar 22, 2023

Ellerbach commented Mar 22, 2023

pgrawehr Mar 25, 2023

joperezr commented Mar 30, 2023

azure-pipelines bot commented Mar 30, 2023

joperezr commented Apr 6, 2023

Adding RPI dtoverlay to check in configuration file #2058

Adding RPI dtoverlay to check in configuration file #2058

Conversation

Ellerbach commented Mar 20, 2023 • edited by ghost

Microsoft Reviewers: Open in CodeFlow

joperezr commented Mar 20, 2023

pgrawehr commented Mar 20, 2023

Ellerbach commented Mar 21, 2023

krwq Mar 21, 2023

Choose a reason for hiding this comment

Ellerbach Mar 21, 2023

Choose a reason for hiding this comment

Ellerbach Mar 21, 2023

Choose a reason for hiding this comment

krwq left a comment

Choose a reason for hiding this comment

Ellerbach commented Mar 21, 2023

pgrawehr commented Mar 22, 2023

Ellerbach commented Mar 22, 2023

pgrawehr Mar 25, 2023

Choose a reason for hiding this comment

joperezr commented Mar 30, 2023

azure-pipelines bot commented Mar 30, 2023

joperezr commented Apr 6, 2023

Ellerbach commented Mar 20, 2023 •

edited by ghost