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

Rework pinmap for each platforms #1838

Merged
merged 9 commits into from Aug 2, 2019

Conversation

@YutingYou
Copy link
Contributor

commented Jun 26, 2019

Problem

We use conditional compilation to reuse the pin definition which makes our pinmap messy. The pin definition should be moved to platform-specific implementation headers.

Pin information STM32_Pin_Info and NRF5X_Pin_Info are replaced by Hal_Pin_Info, we'll abandon the platform-related naming to get rid of some workarounds such as typedef NRFX_Pin_Info STM32_Pin_Info.

Steps to Test

The old pin definition is fragmented, if you want to review the pin definition, you can utilize VSCode highlight function by adding two lines platform definition to compare.

#include "platforms.h"
#define PLATFORM_ID PLATFORM_XSOM 

image

References

[CH34916]


Completeness

  • [internal] Refactors platform pinmap to be in platform-specific headers

@YutingYou YutingYou requested a review from avtolstoy Jun 26, 2019

hal/inc/pinmap_hal.h Show resolved Hide resolved
hal/src/core/pinmap_hal.c Outdated Show resolved Hide resolved
hal/src/gcc/pinmap_impl.h Show resolved Hide resolved
hal/src/gcc/pinmap_impl.h Outdated Show resolved Hide resolved
hal/src/nRF52840/pinmap_hal.c Outdated Show resolved Hide resolved
hal/src/nRF52840/pinmap_impl.h Outdated Show resolved Hide resolved
hal/src/newhal/pinmap_impl.h Outdated Show resolved Hide resolved
hal/src/stm32f2xx/pinmap_impl.h Outdated Show resolved Hide resolved
hal/src/core/pinmap_impl.h Outdated Show resolved Hide resolved
@technobly

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Will this break a lot of library usage of these functions? I would argue this is our public API for low level pin usage, so if it will break things this should be deprecated in 1.x.x and removed only in 2.x.x. We would also need to fix the warnings being seen in the web IDE.

@avtolstoy

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Will this break a lot of library usage of these functions?

Which functions? We are not introducing any breaking changes. This is mostly just a reorganization of where the pin names and internal structures come from: from per-platform hal source folder.

hal/src/electron/pinmap_hal.c Outdated Show resolved Hide resolved
hal/src/xenon/pinmap_defines.h Outdated Show resolved Hide resolved
@avtolstoy

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

@YutingYou Please rebase on current develop and let's merge this.

@YutingYou YutingYou force-pushed the feature/pinmap-rework branch from 53120ee to bdf48bc Aug 2, 2019

@YutingYou YutingYou merged commit 4d4ae62 into develop Aug 2, 2019

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.