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

[WIP] Added Driver for Compute Module 3 #765

Draft
wants to merge 3 commits into
base: master
from

Conversation

@mjjames
Copy link

commented Oct 3, 2019

Fixes #762

Updated the RPI3 Driver to use the PinCount for it's validation so that we can then override just this value for the new Compute Module 3 Driver.

Added tests and updated tool to include the new driver

@mjjames

This comment has been minimized.

Copy link
Author

commented Oct 3, 2019

I'm having issues building on the PI directly, If I build via Ubuntu on WSL everything is fine and the compiled output runs on my devices however if I build on a PI I get the following errors:

error : Failed to load ���� ��v, error:
/home/pi/iot/.dotnet/shared/Microsoft.NETCore.App/2.1.11/libhoolicy.so: wrong ELF class: ELFCLASS64 [/home/pi/iot/src/devices/Media/Media.csproj]
/home/pi/.nuget/packages/microsoft.net.compilers.toolset/3.1.0-beta1-19127-06/tasks/netcoreapp2.1/Microsoft.CSharp.Core.targets(58,5): error : An error occurred while loading required library libhostpolicy.so from [/home/pi/iot/.dotnet/shared/Microsoft.NETCore.App/2.1.11] [/home/pi/iot/src/devices/Media/Media.csproj

Should I only be building on Ubuntu and then deploy artifacts to the device? Or is there an issue with the beta toolsets?

Thanks

-Update-
I forgot to add running this on a clean install of Raspbian Stretch

@mjjames

This comment has been minimized.

Copy link
Author

commented Oct 3, 2019

I hadn't realised the tests would auto mate in this way, what's the best way forward on this test?

@mjjames

This comment has been minimized.

Copy link
Author

commented Oct 3, 2019

@joperezr can you advise on this?

@mjjames mjjames changed the title Added Driver for Compute Module 3 [WIP] Added Driver for Compute Module 3 Oct 3, 2019

namespace System.Device.Gpio.Tests
{
public class RaspberryPiComputeModuleDriverTests : GpioControllerTestBase

This comment has been minimized.

Copy link
@joperezr

joperezr Oct 4, 2019

Member

Adding System.Device.Gpio tests is a bit complex since we actually run these on actual hardware so we rely on things being connected in the right way in order for tests to be able to run correctly. Because of that reason, let's hold off on adding any tests for this for now, and let's first get the implementation working, and then we can add the tests later after connecting the devices properly in the lab.

}
private void ValidatePinNumber(int pinNumber)
{
if (pinNumber < 0 || pinNumber >= PinCount)

This comment has been minimized.

Copy link
@joperezr

joperezr Oct 4, 2019

Member

Please fix your environment so that it uses whitespaces instead of tabs, most of the diffs in your PR are simply changes between tabs and whitespaces.

This comment has been minimized.

Copy link
@mjjames

mjjames Oct 4, 2019

Author

apologies on this, i had assumed the .editorconfig file had handled this, I'll see why this happened and correct

{
public class RaspberryPiComputeModule3Driver : RaspberryPi3Driver
{
protected internal override int PinCount => 46;

This comment has been minimized.

Copy link
@joperezr

joperezr Oct 4, 2019

Member

Is the only difference between the ComputeModule3 driver and the raspberryPi driver the number of pins it exposes? If so, I'm not sure if this would grant the requirement of adding a whole new driver for it.


private static GpioDriver GetBestDriverForRaspberryPiRevision(string revisionLine)
{
Regex regex = new Regex(@"Revision\s*:\s*(.*)");

This comment has been minimized.

Copy link
@joperezr

joperezr Oct 4, 2019

Member

We really want to limit the use of Regex as much as possible, we are already doing it above for getting the best driver for the board but we are looking for a better way to do this, so I would be hesitant to add yet another place where we use Regex. In general, Regex tends to decrease performance of the app and it might also be a cause security issues, so we try to avoid it unless it is absolutely needed.

This comment has been minimized.

Copy link
@mjjames

mjjames Oct 4, 2019

Author

I agree on the use of Regex, I was following what had been done previously. Based on other issues, reading the Revision from the cpuinfo is very Raspbian focused anyway so it's probably better to tackle the cpuinfo issue first and then reinvestigate this.

@joperezr

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

Before we move forward with this PR, I would like to understand what is the real pain point here. Is it just trying to use this driver for a device that provides extra gpio pins? If so, can we fix this problem differently? For example, we could have people manually pick the UnixDriver instead of the raspberryPi driver which should work, since it also makes me a bit worried the fact that I'm not sure if those extra Gpios are controlled the exact same way as with the RaspberryPi 3. We would have to read the datasheet since with the RaspberryPi3 driver we are accessing registers directly, which have a very specific address in memory, and I'm not sure if these extra pins would be represented in the same way, or if they would be located in the same location in memory.

@mjjames

This comment has been minimized.

Copy link
Author

commented Oct 4, 2019

To clarify the position. Essentially yes additional pin's is the issue. We use the Compute Module 3's and these provide 3 banks of GPIO, 0, 1 & 2. GPIO 2 is reserved for eMMC so is not an issue. GPIO 0 (I'm fairly certain although not 100%) is very similar to the RPI 3. GPIO 1 is then the additional range of pins that is not exposed.

I started this prior to the recent change with the register changes so haven't tested with that yet. My device testing of simply upping the maximum GPIO pin number has been working but I haven't fully tested every pin, only simple reading and writing.

If it's more advisable I can update our calling code to pass in the UnixDriver, I'm just also thinking how by default the Compute Module 3 falls into the RaspberryPI3 Driver via the default constructor.

I had considered whether to provide a constructor overload on the RaspberryPi3 driver that could enable the additional GPIO Bank 1 pins, or even have the driver auto detect its a compute module and enable them but this would still need to consider the above comments regarding registers.

What would be the next suitable steps?

@joperezr

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

Essentially yes additional pin's is the issue. We use the Compute Module 3's and these provide 3 banks of GPIO, 0, 1 & 2. GPIO 2 is reserved for eMMC so is not an issue. GPIO 0 (I'm fairly certain although not 100%) is very similar to the RPI 3. GPIO 1 is then the additional range of pins that is not exposed.

I see, that makes sense. Thanks for clarifying. Also I do agree that having the RaspberryPi3 being the default driver that gets picked is not ideal in the case this driver won't be able to access all the functionality of the device. I liked the idea you proposed about simply modifying the RaspberryPi3 existing driver to detect if we are running in this specific device, and if so, changing how many pins would it provide.

The next steps in my mind would be to read the Compute Module 3's datasheet to ensure that all registers we use are located in the same address and that we will be able to access the new additional ones with the same logic that we have. I do recall when I initially implemented the driver that there were a few bits on the GPIO registers that were unused as I believe that two ints were used to represent the values of Gpios (1 bit per pin, which gave us 64) so given this special pi has 46 available pins, they would still fit in the same registers. That said other stuff may have shift, so lets first ensure that all of the addresses match and pins are controlled in the same way, and if so we can start planning on how to add support for this. Does that sound like a plan?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.