Skip to content

board/nucleo-l412kb#12144

Closed
mrusme wants to merge 3 commits into
RIOT-OS:masterfrom
mrusme:board_nucleo-l412kb
Closed

board/nucleo-l412kb#12144
mrusme wants to merge 3 commits into
RIOT-OS:masterfrom
mrusme:board_nucleo-l412kb

Conversation

@mrusme

@mrusme mrusme commented Sep 2, 2019

Copy link
Copy Markdown

Contribution description

Implemented Nucleo L412KB support. The code is compiling, but it's untested yet as I have to figure out how to get past an open-ocd issue that I'm experiencing:

### Flashing Target ###
Open On-Chip Debugger 0.10.0
Licensed under GNU GPL v2
For bug reports, read
	http://openocd.org/doc/doxygen/bugs.html
hla_swd
Info : The selected transport took over low-level target control. The results might differ compared to plain JTAG/SWD
adapter speed: 500 kHz
adapter_nsrst_delay: 100
none separate
srst_only separate srst_nogate srst_open_drain connect_deassert_srst
Info : Unable to match requested speed 500 kHz, using 480 kHz
Info : Unable to match requested speed 500 kHz, using 480 kHz
Info : clock speed 480 kHz
Info : STLINK v2 JTAG v31 API v2 SWIM v21 VID 0x0483 PID 0x374B
Info : using stlink api v2
Info : Target voltage: 3.239843
Info : stm32l4x.cpu: hardware has 6 breakpoints, 4 watchpoints
    TargetName         Type       Endian TapName            State
--  ------------------ ---------- ------ ------------------ ------------
 0* stm32l4x.cpu       hla_target little stm32l4x.cpu       halted
Info : Unable to match requested speed 500 kHz, using 480 kHz
Info : Unable to match requested speed 500 kHz, using 480 kHz
adapter speed: 480 kHz
target halted due to debug-request, current mode: Thread
xPSR: 0x01000000 pc: 0x08004604 msp: 0x200022d0
auto erase enabled
Info : device id = 0x10006464
Warn : Cannot identify target as a STM32L4 family.
Error: auto_probe failed

make: *** [flash] Error 1

Issues/PRs references

#12139

@mrusme mrusme mentioned this pull request Sep 2, 2019
@benpicco

benpicco commented Sep 2, 2019

Copy link
Copy Markdown
Contributor

I have to figure out how to get past an open-ocd issue that I'm experiencing:
Open On-Chip Debugger 0.10.0

The last OpenOCD release is from January 2017 and lacks support for many new chips. Did you try building the latest OpenOCD directly from git master?
This did also bite me for same54 for which the latest stable release also lacks support, but the master branch has it.

@aabadie

aabadie commented Sep 2, 2019

Copy link
Copy Markdown
Contributor

From your OpenOCD log:
Warn : Cannot identify target as a STM32L4 family.
It seems your OpenOCD doesn't support this CPU. I haven't check but maybe it's not part of OpenOCD or there's a pending patch waiting to be merged.

@mrusme

mrusme commented Sep 2, 2019

Copy link
Copy Markdown
Author

Thanks for the hints! I ran brew install open-ocd --HEAD and got what appears to be the latest master (0819541). After running it with that version, I got the same issue and there was a warning: WARNING: interface/stlink-v2-1.cfg is deprecated, please switch to interface/stlink.cfg. Hence I adjusted https://github.com/RIOT-OS/RIOT/blob/master/makefiles/tools/openocd-adapters/stlink.inc.mk#L7 to -c 'source [find interface/stlink.cfg]' \. The warning disappeared, the issue still remained.

I've then investigated the open-ocd source and digged into /src/flash/nor/stm32l4x.c. It doesn't include the correct settings for device_id 0x10006464, so I added the following lines:

        case 0x464:
                max_flash_size_in_kb = 128;
                break;

Screenshot 2019-09-02 at 12 02 24 PM

Then I basically compiled the code manually using:

# ./bootstrap nosubmodule
# ./configure --prefix=/usr/local/Cellar/open-ocd/HEAD-0819541 --enable-buspirate --enable-stlink --enable-dummy --enable-jtag_vpi --enable-remote-bitbang
# make install

(I used the same prefix as brew did since I have hopes for the open-ocd project to include these changes at some point, which would then overwrite my hack as a brew bottled installation)

Using this updated version I could finally flash the code and test the hello-world example:

Screenshot 2019-09-02 at 12 07 06 PM

So I guess this PR could be tested and merged, if anyone feels like going through this procedure. 🤷‍♂ And no, I haven't opened an issue on open-ocd about this, because, SourceForge.

@benpicco

benpicco commented Sep 2, 2019

Copy link
Copy Markdown
Contributor

Nice! But hey, they even have a nice guide: http://openocd.org/doc-release/doxygen/patchguide.html

@mrusme

mrusme commented Sep 2, 2019

Copy link
Copy Markdown
Author

@benpicco initially I just wanted to replace the lightbulb in the kitchen, you know?

t0XHtgJ

@benpicco benpicco added the Area: boards Area: Board ports label Sep 2, 2019
@aabadie

aabadie commented Sep 2, 2019

Copy link
Copy Markdown
Contributor

I think someone already tries to add support to this cpu to OpenOCD: http://openocd.zylin.com/#/c/4934/

Regarding stlink version config file in OpenOCD, it's just a warning and it should still work with the old version. Enabling the generic stlink config will break flash when one uses OpenOCD 0.10 which is the latest release. So I won't include this change in this PR.

@mrusme

mrusme commented Sep 2, 2019

Copy link
Copy Markdown
Author

@aabadie thought about checking for the $(CPU) and deciding which to use, based on that info, but no worries, I didn't include it. If anything we can do that as separate PR.

Comment thread cpu/stm32_common/periph/gpio.c Outdated
@benpicco

benpicco commented Sep 2, 2019

Copy link
Copy Markdown
Contributor

This is looking good, but one nit: Please split this into a commit to add support for the cpu (cpu/stm32l4: add support for STM32L412KB) and one for the board (boards: add support for nucleo-l412kb) so things are nice and consistent in the git history.

@mrusme

mrusme commented Sep 2, 2019

Copy link
Copy Markdown
Author

@benpicco 👍

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Sep 2, 2019
Comment thread boards/nucleo-l412kb/Makefile.features Outdated
@mrusme

mrusme commented Sep 3, 2019

Copy link
Copy Markdown
Author

@benpicco might wanna check again? Not sure if it's implemented in the best way possible now, but the board seems to work good so far.

@benpicco

benpicco commented Sep 3, 2019

Copy link
Copy Markdown
Contributor

Murdock says no

Murdock is not happy. You could either remove advertising support for the features that are not implemented yet for this board/CPU - or you could go about implementing them. 😉

Comment thread cpu/stm32l4/include/vendor/stm32l412xx.h Outdated
@benpicco benpicco requested a review from fjmolinas September 17, 2019 22:18
@fjmolinas

Copy link
Copy Markdown
Contributor

Murdock is complaining because the tests do not fit in ROM, please blacklist the board from those tests by adding it to BOARD_INSUFFICIENT_MEMORY in the test Makefile.

It would be useful if you could run all tests for the board by doing, so we can see it is working properly even though we do not have the board to test.

RIOT/dist/tools/compile_and_test_for_board/compile_and_test_for_board.py .nucleo-l412kb --with-test-only

If possible please post the results of this, it should take between 30min~1hr depending on your cpu.

@fjmolinas fjmolinas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have just one minor comment in code.

#endif

#if defined(CPU_MODEL_STM32L412KB)
#define RTC_ISR_ALRAF RTC_SR_ALRAF

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Although I prefer this indentation, it is not the way it is done for the rest of the code, so please change this to:

#if defined(CPU_MODEL_STM32L412KB)
#define RTC_ISR_ALRAF RTC_SR_ALRAF
#endif

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think

#ifndef RTC_ISR_ALRAF 
#define RTC_ISR_ALRAF RTC_SR_ALRAF
#endif 

would be a more generic solution.

return -1;
}

#if defined(CPU_MODEL_STM32L412KB)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When in Rome, do as the Roman's do.

I don't like editing vendor files, but having this be the only member of that family that didn't edit that part of the vendor files makes it odd too.

I don't see a generic way to detect this either, so let's just follow the precedent and also edit stm32l412xx.h to change CCR1…4 -> CCR[4].

Then the changes to common files should be minimal and this will be good to merge.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with you @benpicco. It's not nice to edit CMSIS files but since this is done like this for others...

@benpicco

Copy link
Copy Markdown
Contributor

You don't have to update all the BOARD_INSUFFICIENT_MEMORY lists by hand - #12485 contains a script that does it all automatically.

@benpicco

Copy link
Copy Markdown
Contributor

You can just run dist/tools/insufficient_memory/add_insufficient_memory_board.sh nucleo-l412kb to update all the Makefile.cis for the new board.

Then you just have to address some small comments and this should be good 😉

@mrusme

mrusme commented Nov 20, 2019

Copy link
Copy Markdown
Author

@benpicco thank you for the feedback, really appreciate your effort! :-) Sorry for the long delay, unfortunately I currently have a few other topics on my hands, but lightbulb-changing should soon be back to normal so that I can finally finish this PR. :-) Simply haven't had time so far.

@aabadie aabadie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have the board so I can test this PR.

Support for this CPU was added in openocd recently, so no more blocker for adding this board. @mrusme could you address the remaining comments ?

return -1;
}

#if defined(CPU_MODEL_STM32L412KB)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with you @benpicco. It's not nice to edit CMSIS files but since this is done like this for others...

@aabadie

aabadie commented Mar 17, 2020

Copy link
Copy Markdown
Contributor

I tested this PR and could make it work after some changes. But I had some problems:

  • it doesn't build when rebased on master
  • tests/periph_rtc doesn't work
  • the board must be blacklisted on some new tests

@mrusme, I made some changes locally but I can't PR or push to your branch. Are you ok if I take over this PR ? I hope we'll be able to get it merged sooner.

@mrusme

mrusme commented Mar 17, 2020

Copy link
Copy Markdown
Author

@aabadie sorry, go ahead!

@aabadie

aabadie commented Mar 19, 2020

Copy link
Copy Markdown
Contributor

Thanks @mrusme, here it is: #13660. Normally your initial commits are kept.

I'm closing this one.

@aabadie aabadie closed this Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants