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

Separate out low level GPIO #1485

Merged
merged 15 commits into from
Nov 20, 2019
Merged

Separate out low level GPIO #1485

merged 15 commits into from
Nov 20, 2019

Conversation

AdrianSoundy
Copy link
Member

@AdrianSoundy AdrianSoundy commented Nov 6, 2019

Description

I have created this PR for ESP32 so I can get some feedback on this change before doing any more work on it and implementing for other devices.

Separate out the GPIO so that interface can be reused by other devices instead of device going directly to low level calls. Similar to Netmf but not the same.

Advantages

  • This allows better sharing of devices.
  • The device specific code can be moved to common code reducing new device porting effort.
  • Reduce memory usage if multiple devices using it.

This is currently needed for the ENC28j60 Ethernet and the Graphic touch interface. Other existing devices can also be ported to use it.

This is the first device to be done with SPI being next as also used by ENC28j60 & Graphic driver.

Problem areas:-
The current code uses an array to save all the working variables for each gpio pin where before it saved data in the managed code. This reserves memory for each possible pin even if not used.

Possible solutions:-

  • Use a map for storing working data but that adds a large chuck of library code, ok if used else where.
  • Use some other structure ?
  • Pass in ptr to the working area when pin enabled. It can then be decided by calling code where this is saved.

The reserved bool would have to be moved out of current array, made into a bit array.

The current code uses a common bounce timer which I can see would be a problem if more than 1 gpio was using debounce. i.e example uses Button and a limit switch.

Should we rename the CPU_GPIO_decl.h & cpu_gpop.cpp to something more nanoFramework ?

Motivation and Context

Needed to move forward with other devices

How Has This Been Tested?

ESP32 has had some testing but not extensive.

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: adriansoundy [email protected]

@nfbot
Copy link
Member

nfbot commented Nov 6, 2019

Hi @AdrianSoundy,

I'm nanoFramework bot.
Thank you for your contribution!

A human will be reviewing it shortly. 😉

@AdrianSoundy AdrianSoundy added Series: ESP32 Everything related specifically with ESP32 series targets Area: Interpreter Everything related with the interpreter, execution engine and such Breaking-change Status: in progress and removed Series: ESP32 Everything related specifically with ESP32 series targets labels Nov 6, 2019
Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

I can see value of this change. Having a common code base to handle the general stuff and leaving to the PAL layer the hardware specifics sounds good to me.
Also being able to manage if a GPIO pin is already in use adds reliability to the platform. Right now this is not managed and can lead to overlapping use.

The obvious draw backs are:

  1. Adds more code (still struggling with shrinking the code base to make if fit into small MCUs and we are about to increase it).
  2. Affects performance. The most common benchmark for this context is the infamous GPIO toggle frequency. This would add even more delay to execute such a "simple" task.

Thoughts/questions:

  1. If the gpio_state array wasn't initialized at boot but instead the state blocks are added (with malloc) and free when removed this wouldn't waste RAM but would use only what's required.
  2. A linked list could be used instead of the fixed array to manage the state. Reuse the existing DBlinkedList (C++) or use a plain C linked list (preferable if we are to pursue the goal to keep porting the code base to C in order to drop C++).
  3. I guess this would make other libraries which are dependent of GPIO (PWM, I2C, etc) to have to reference the new GPIO, correct?

@AdrianSoundy
Copy link
Member Author

AdrianSoundy commented Nov 12, 2019

Thanks for your last feedback

I had a look at firmware sizes and for ESP32 it added about a 1K over develop branch version but i also added handling for de-bounce logic so some of that would account for that.

Also looked at speed when adding another layer. For ESP32 was around 22hz slower between current firmware and this one with a toggle loop. I have optimised some calling code so maybe its the same again. Did look at using inline method for CPU_GPIO_SetPinState() but couldn't get it to compile.

Now implemented a DBlinkedList for the storing the input states and separated the reserve into a bit array. Also done some more tidying up.

I have implemented the STM32 version of cpu_gpio.ccp and now the files:-

  • win_dev_gpio_native_Windows_Devices_Gpio_GpioPin.cpp
  • win_dev_gpio_native_Windows_Devices_Gpio_GpioController.cpp
    are common in ESP32 & STM32 builds but still in same place.

Hope to move these to a common place when all targets are done.

I haven't tested the gpio on STM32 target yet but it does build.

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

Looking good!

@AdrianSoundy
Copy link
Member Author

Tested on F746ZG and seems to be working ok with a couple of Output Gpio(led) and a couple of input buttons

@AdrianSoundy
Copy link
Member Author

I have done the NXP version now but this is very much untested as i had to add all the interrupt handling. If you are happy with whats been done already then i suggest this is merged and i will do the NXP gpio on a separate PR so it can be tested.

@josesimoes
Copy link
Member

Got you. Let me ask @MateuszKlatecki input on this.
As for merging it, let me test this on a couple of STM and TI boars. Unless you've already done it...

@AdrianSoundy
Copy link
Member Author

AdrianSoundy commented Nov 18, 2019

I have just tested on ESP32 & STM32 F746ZG, Ti should be done.
Spoke to @MateuszKlatecki and they are happy to test NXP changes(When done)

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

LGTM

@josesimoes josesimoes added Area: Common libs Everything related with common libraries and removed Area: Interpreter Everything related with the interpreter, execution engine and such Status: in progress Status: under review labels Nov 20, 2019
@josesimoes josesimoes merged commit 7d43495 into nanoframework:develop Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Common libs Everything related with common libraries Breaking-change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants