Add explicit pin-map support for extra targets #11632

mprse · 2019-10-04T12:08:21Z

Description

This PR is the continuation of PR #11399.

Adds explicit pinmap support for the following boards:

NUCLEO_F411RE
DISCO_L475VG_IOT01A
NRF52840_DK
NUCLEO_F303RE
LPC55S69_NS
NUCLEO_L073RZ

Adds support for the peripherals:

CAN
QSPI

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@jamesbeyond @kjbracey-arm @0xc0170 @c1728p9
@maciejbocianski @fkjagodzinski

ciarmcom · 2019-10-04T13:00:24Z

@mprse, thank you for your changes.
@fkjagodzinski @maciejbocianski @kjbracey-arm @jamesbeyond @c1728p9 @0xc0170 @ARMmbed/mbed-os-maintainers please review.

adbridge · 2019-10-07T15:15:41Z

@mprse please remove reference to golden boards

mprse · 2019-10-08T06:30:49Z

Rebased this PR on the top of feature-hal-spec-explicit-pinmap after PR #11399 has been merged.

This PR is ready for review/CI.

@adbridge @jamesbeyond I noticed that PR name has been changed. Actually the name I think was ok. This one adds support for the remaining golden targets + NUCLEO_L073RZ. But the current name is also ok.

mprse · 2019-10-11T09:03:55Z

This one waits for the review... Can we run CI meanwhile to check and fix potential failures?

mprse · 2019-10-16T06:36:59Z

@0xc0170 Can we run CI?

0xc0170 · 2019-10-18T13:30:22Z

Travis restarted, CI started

mbed-ci · 2019-10-18T15:46:45Z

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_build-GCC_ARM

0xc0170 · 2019-10-21T09:09:54Z

Build failures related to the change-set, please review

mprse · 2019-10-21T13:50:26Z

Build failures related to the change-set, please review

@0xc0170 Fixed build errors. Can we try again?

0xc0170 · 2019-10-21T13:52:05Z

CI restarted

mbed-ci · 2019-10-21T17:19:35Z

Test run: FAILED

Summary: 1 of 12 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

mprse · 2019-10-22T07:33:03Z

Test run: FAILED

One test has failed:
K64F GCC_ARM / allocation failure – K64F-GCC_ARM.tests-mbed_platform-stats_heap

This might be related. @0xc0170 have you seen such failures?
I tried to run this test locally and it looks like it passes all the time. Maybe we can try once more to confirm that this is related to this PR?

0xc0170 · 2019-10-22T07:48:28Z

Tests restarted

kjbracey · 2019-10-22T09:09:05Z

That looks like the bug fixed by #11572. Rebase if your branch doesn't have that fix.

mprse · 2019-10-22T12:06:15Z

Rebased this PR after the target branch has been rebased.
Ready for the next CI round.

0xc0170 · 2019-10-22T13:31:37Z

CI started

mbed-ci · 2019-10-22T16:07:45Z

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 3
Build artifacts

mprse · 2019-10-23T06:13:37Z

CI passed, still waiting for review.
When this one is merged and the documentation(PR #11569) I can bring explicit pinmap extension into the master.

kjbracey · 2019-10-23T07:33:35Z

drivers/QSPI.h

@@ -222,13 +234,15 @@ class QSPI : private NonCopyable<QSPI> {
    int _mode; //SPI mode
    bool _initialized;
    PinName _qspi_io0, _qspi_io1, _qspi_io2, _qspi_io3, _qspi_clk, _qspi_cs; //IO lines, clock and chip select
+    const qspi_pinmap_t *_explicit_pinmap;


Are you requiring the passed-in pinmap object to have lifetime extending beyond the constructor call? That's not specified.

~~Looking at the code, I don't think you are, so this doesn't have to be a member. You could just pass the pinmap to the _initialize_direct. Will save 4 bytes of RAM.~~ (Nope - you should be calling _initialize_direct other places)

I realise this model comes from the SPI code, which does require the pinmap to persist for later re-init. That one does need that specified. Maybe the same should be specified for all classes to allow suspend/resume in future, even if they don't do it now.

Pondering the persistence thing - this means that

SPI(get_spi_pinmap(A,B,C,D))

which I've seen suggested is actually illegal. That would be retaining a pointer to a temporary.

It's unfortunate that the compiler accepts it. You're allowed to pass a temporary X to a const X &

Two suggestions to prevent that here: https://stackoverflow.com/questions/40790126/prevent-passing-temporary-for-const-ref-parameter

Delete the overload with r-value parameter: SPI(const spi_pinmap_t &&) = delete

Use const pointer: SPI(const spi_pinmap_t *)

Not sure it's too worth worrying about.

The mentioned form SPI(get_spi_pinmap(A,B,C,D)) is actually not suggested. According to the design doc:

It is strongly suggested that the result of get_xxx_pinmap() calls should normally be used in explicitly constant expression contexts like initializing a constexpr variable, to guarantee that the tables are not pulled in.

I think we should prevent passing of temporary objects. The first option looks more suitable for our case.

I added
SPI(const spi_pinmap_t &&, PinName) = delete;

And now when SPI spi(get_spi_pinmap(PA_7, PA_6, PA_5, PA_4)); causes compilation error:

[Error] main.cpp@41,9: call to deleted constructor of 'mbed::SPI' [ERROR] .\main.cpp:41:9: error: call to deleted constructor of 'mbed::SPI' SPI spi(get_spi_pinmap(PA_7, PA_6, PA_5, PA_4)); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./drivers/SPI.h:146:5: note: 'SPI' has been explicitly marked deleted here SPI(const spi_pinmap_t &&) = delete; ^ 1 error generated.

This is what we wanted to achieve. The last question is if this solution should be added only to SPI and QSPI classes or to all peripheral constructors (which support explicit pinmap)?

At the minute, the others do support passing a temporary, I guess, because they copy all the data out in the constructor.

Removing that ability to pass a temporary later (eg if they wanted to implement suspend/resume) would be an API change, so I guess it's probably better to ensure it's blocked now. And it prevents the bad code generation possibility.

kjbracey · 2019-10-23T07:35:58Z

drivers/source/CAN.cpp

+{
+    // No lock needed in constructor
+
+    for (size_t i = 0; i < sizeof _irq / sizeof _irq[0]; i++) {


This loop doesn't do anything? You're already initialising _irq with the _irq() in the initializer-list. Guess you copied it from the others. Scrub it.

(Don't even actually need the _irq(), as it's an array of Callbacks with default constructors, just as you don't need a _mutex()).

kjbracey · 2019-10-23T07:38:46Z

drivers/source/QSPI.cpp

@@ -255,6 +281,24 @@ bool QSPI::_initialize()
    return _initialized;
 }

+// Note: Private helper function to initialize qspi HAL
+bool QSPI::_initialize_direct()


_acquire is still just calling _initialize - don't you need to do the _init_func thing that SPI had?

QSPI constructor calls HAL init() function, but I missed that _acquire function also calls init(). You are right that the same mechanism as in SPI must be also used here.

kjbracey · 2019-10-23T07:40:50Z

hal/explicit_pinmap.h

+        }
+    }
+
+    if (!data0_map || !data1_map || !data2_map || !data3_map || !sclk_map || ssel_map || data0_map->peripheral != data1_map->peripheral || data0_map->peripheral != data2_map->peripheral  || data0_map->peripheral != data3_map->peripheral || data0_map->peripheral != sclk_map->peripheral || data0_map->peripheral != ssel_map->peripheral) {


!ssel_map, surely?

mprse · 2019-10-24T11:32:24Z

@kjbracey-arm Thanks for the review. I addressed all the review comments.

mprse · 2019-10-25T06:56:34Z

Fixed style.

0xc0170 · 2019-10-28T14:07:39Z

CI restarted

mbed-ci · 2019-10-28T20:07:47Z

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM

mprse · 2019-10-29T07:15:50Z

Test run: FAILED

Not sure but it looks like some CI issue. Few boards in Jenkins are marked with an exclamation mark with info passed in 0 s, but marked as unstable.

0xc0170 · 2019-10-29T09:01:40Z

CI restarted

mbed-ci · 2019-10-29T14:22:16Z

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 5
Build artifacts

mprse · 2019-10-29T14:28:27Z

CI passed. @kjbracey-arm All change requests were fixed. Unless there are no other objections this PR is ready for merge.

0xc0170 · 2019-10-29T14:50:22Z

As this is for the feature branch, should be good to go now.

mprse changed the base branch from master to feature-hal-spec-explicit-pinmap October 4, 2019 12:08

mprse force-pushed the explicit_pinmap_gold_2 branch from 6b7f804 to cb04609 Compare October 4, 2019 12:11

ciarmcom requested review from 0xc0170, c1728p9, fkjagodzinski, jamesbeyond, kjbracey, maciejbocianski and a team October 4, 2019 13:00

ciarmcom added the needs: review label Oct 4, 2019

mprse mentioned this pull request Oct 7, 2019

Add explicit pin-map support for K64F and NUCLEO_F429ZI #11399

Merged

jamesbeyond changed the title ~~Add explicit pin-map support for the remaining golden targets~~ Add explicit pin-map support for extra targets Oct 7, 2019

mprse force-pushed the explicit_pinmap_gold_2 branch from cb04609 to 8c328ad Compare October 8, 2019 06:24

c1728p9 approved these changes Oct 15, 2019

View reviewed changes

0xc0170 added the release-version: feature branch label Oct 18, 2019

mprse force-pushed the explicit_pinmap_gold_2 branch from 8c328ad to 57c6aa0 Compare October 21, 2019 13:49

NUCLEO_F429ZI add CAN pinmaps

82aefe7

mprse force-pushed the explicit_pinmap_gold_2 branch from 57c6aa0 to 82aefe7 Compare October 22, 2019 12:05

kjbracey reviewed Oct 23, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Oct 24, 2019

mprse added 4 commits October 24, 2019 13:02

QSPI: Add pointer to HAL init function

21a9712

get_qspi_pinmap(): Fix invalid condition

e048619

CAN: remove redundant loop

ba95fcf

prevent passing of temporary pinmap objects

9813021

mprse requested a review from kjbracey October 24, 2019 11:32

Serial, QSPI: Fix style

d38c7d6

0xc0170 added needs: CI and removed needs: work labels Oct 28, 2019

0xc0170 added ready for merge and removed needs: CI labels Oct 29, 2019

0xc0170 merged commit afa7630 into ARMmbed:feature-hal-spec-explicit-pinmap Oct 29, 2019

0xc0170 removed the ready for merge label Oct 29, 2019

Add explicit pin-map support for extra targets #11632

Add explicit pin-map support for extra targets #11632

Uh oh!

Conversation

mprse commented Oct 4, 2019

Description

Pull request type

Reviewers

Uh oh!

ciarmcom commented Oct 4, 2019

Uh oh!

adbridge commented Oct 7, 2019

Uh oh!

mprse commented Oct 8, 2019

Uh oh!

mprse commented Oct 11, 2019

Uh oh!

mprse commented Oct 16, 2019

Uh oh!

0xc0170 commented Oct 18, 2019

Uh oh!

mbed-ci commented Oct 18, 2019

Uh oh!

0xc0170 commented Oct 21, 2019

Uh oh!

mprse commented Oct 21, 2019

Uh oh!

0xc0170 commented Oct 21, 2019

Uh oh!

mbed-ci commented Oct 21, 2019

Uh oh!

mprse commented Oct 22, 2019

Uh oh!

0xc0170 commented Oct 22, 2019

Uh oh!

kjbracey commented Oct 22, 2019

Uh oh!

mprse commented Oct 22, 2019

Uh oh!

0xc0170 commented Oct 22, 2019

Uh oh!

mbed-ci commented Oct 22, 2019

Uh oh!

mprse commented Oct 23, 2019

Uh oh!

kjbracey Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprse Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprse commented Oct 24, 2019

Uh oh!

mprse commented Oct 25, 2019

Uh oh!

0xc0170 commented Oct 28, 2019

Uh oh!

mbed-ci commented Oct 28, 2019

Uh oh!

mprse commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey Oct 23, 2019 •

edited

Loading

mprse Oct 23, 2019 •

edited

Loading

mprse commented Oct 29, 2019 •

edited

Loading

mprse commented Oct 29, 2019 •

edited

Loading