lpc: Add basic micropython port to LPC Cortex-M series microcontrollers #3400
Conversation
First board introduced is EDU CIAA LPC4337 (educational board), using LPCOpen library for LPC18xx/43xx v3.01 as external module. This first commit should introduce some infrastructure for other boards to be easily supported. For now, let's just enable a very simple REPL. Signed-off-by: Ernesto Gigliotti <ernestogigliotti@gmail.com> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Signed-off-by: Martin Ribelotta <martinribelotta@gmail.com>
There was a problem hiding this comment.
@martinribelotta Great job reducing the diffstat! It looks really nice. I just had a few style comments here and there.
Also, a question: what's your plan to select between boards? I see that ports/lpc/board.h has some board-specific macros. Perhaps we should include a per-board header instead, and declare those macros there?
On the other side, that can be done on a follow-up pull-request.
@dpgeorge What do you think?
| const uint32_t OscRateIn = 12000000; | ||
|
|
||
| /* Structure for initial base clock states */ | ||
| struct CLK_BASE_STATES { |
There was a problem hiding this comment.
why capital letters for the struct?
|
|
||
| /* Structure for initial base clock states */ | ||
| struct CLK_BASE_STATES { | ||
| CHIP_CGU_BASE_CLK_T clk; /* Base clock */ |
There was a problem hiding this comment.
a nitpick: the comment spacing is odd. can we make them all consistent?
CHIP_CGU_CLKIN_T clkin; /* Base clock source, see UM for allowable souorces per base clock */
bool autoblock_enab; /* Set to true to enable autoblocking on frequency change */
bool powerdn; /* Set to true if the base clock is initially powered down */
};
the above snippet has just one space between the semicolong and the beginning of the comment.
|
|
||
| void Board_UART_Init(LPC_USART_T *pUART) | ||
| { | ||
| Chip_SCU_PinMuxSet(0x7, 1, (SCU_MODE_INACT | SCU_MODE_FUNC6)); /* P7.1 : UART2_TXD */ |
There was a problem hiding this comment.
same nitpick comment: a lot of spacing here, vs. no space in the following line.
| Chip_UART_Init(CONSOLE_UART); | ||
| Chip_UART_SetBaudFDR(CONSOLE_UART, 115200); | ||
| Chip_UART_ConfigData(CONSOLE_UART, UART_LCR_WLEN8 | UART_LCR_SBS_1BIT | UART_LCR_PARITY_DIS); | ||
| /* Enable UART Transmit */ |
There was a problem hiding this comment.
more comment nitpicking ;-) no need to have a comment saying "enable UART", given the line below is quite clearly enabling the UART. Comments should only be used to add relevant information, otherwise they just clutter the code.
| /* Setup system level pin muxing */ | ||
| Chip_SCU_SetPinMuxing(pinmuxing, sizeof(pinmuxing) / sizeof(PINMUX_GRP_T)); | ||
| /* Off all leds */ | ||
| #define X(port, pin) do { \ |
There was a problem hiding this comment.
I would just add an inline static function instead of this X macro. gcc should be smart enough to produce the same code. Plus, a function gives you type detection.
martinribelotta
force-pushed
the
to_master4
branch
from
8ef9200
to
db56ebf
Compare
|
@dpgeorge Don't want this to fall into oblivion. Any feedback? |
|
@ezequielgarcia Much of the code is from official BSP of NXP. This need a big rework for a good code guidelinges. The real issue with this PR is the need to add anotjer dependency like stm32lib. This is useless code if you compile for unix or win or stm32 port (or any ports). I try to finding a solution of this but my only reliable path is incorporating only needed functions while the port is growing. @dpgeorge Some sugesstions to this? |
@martinribelotta What do you mean? I think the LPC library will be compiled only if you target LPC. Since micropython is a multiplatform project, I don't think we are gonna get away without having code for multiple platforms. Just like any other multiplatform project: Node.JS, Linux, etc. |
|
@martinribelotta thanks for persisting with this to get the LPC supported.
Do you mean there is code here from NXP code base? What are the licensing issues around that code?
That is ok, having submodules is a way to add ports while keeping the repository relatively lean. But the submodule must be 1) and independent component; 2) not change very often. If your lpc43xxlib submodule is changing too quickly then it won't work because a lot of the commits will just be to update the submodule. |
|
@dpgeorge I reproduce the relevant part of lpcopen licence below:
This terms are incompatible with GPL/LGPL but compatible with MIT/BSD/Apache licence (and presumible others non-copyleft licences) The direct response of NXP support is here: The lpc43lib is very slowly actualized (The change between 2.x and 3.x required 3 years from NXP). Only need to do a little change to support more than one family of processor (lpc43, lpc54xxx, etc) and rename this to nxplib. In the next days I do the pertinent changes and re write the pull request. |
|
@dpgeorge Almost a month has passed. I wonder if there are any huge blockers preventing this patch from being merged. I think we have addressed all of your comments and concerns, and @martinribelotta has already stated the NXP library repo would be updated very infrequently. This micropython port for NXP machines (well, actually a fork based on some ancient micropython) is used in Universities [1] and shown in conferences [2] here in Argentina. Moreover, we are aware of at least a company wanting to enable it on their NXP-based platforms. So, it seems there is enough momentum to justify the adding this support, and enough reasons (in terms of users) to maintain it. Plus, it would make a terrific way of ending 2017 ;-) [1] http://laboratorios.fi.uba.ar/lse/tesis/LSE-FIUBA-Trabajo-Final-CESE-Ernesto-Gigliotti-2016.pdf |
|
Hi. It's been a couple of months without updates and I was wondering what's the current status? |
Hello @dc740! On our side, the PR looks good IIRC (it's been 2 years, so I might be forgetting something). @dpgeorge Do you think you are ready to merge this? Having support for more vendors will surely increase the usefulness of micropython. Also, it seems to do so, as it would have zero impact on the other ports. |
LPC port. Initial commit for LPC43xx support over educational board CIAA-NXP.
This PR clean the board initialization code populating
board.hinterface with common functions for all boards of the port. Additionally, It enable some basic uPy modules like ure, sys, gc, etc.The next plan is to add support to micromint lpc43xx boards and similars.
prev work: #3354
@ezequielgarcia some comments?
@RockySong The support of lpc541xx/lpc546xx is in my roadmap and in the future, the support of iMX-RT. Some suggestions to integrate your work? what is your rouadmap?