TARGET_STM/qspi_api.c: fix build error on bare metal #13617

LDong-Arm · 2020-09-15T16:22:06Z

Summary of changes

mbed-os-example-blinky-baremetal fails to build on DISCO_L475VG_IOT01A (and presumably a few other targets) because TARGET_STM/qspi_api.c assumes mbed-trace to be available, which is not true with the bare metal profile (unless the user enables it in their mbed_app.json). This PR fixes the issue.

Impact of changes

None.

Migration actions required

None.

Documentation

None.

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ARMmbed/mbed-os-hal @ARMmbed/team-st-mcd

…ed-trace

LDong-Arm · 2020-09-15T16:22:41Z

I'm slightly surprised we never caught this issue before. Unless I missed something...

LDong-Arm · 2020-09-15T16:26:57Z

targets/TARGET_STM/qspi_api.c

+#else
+#define tr_info(...)
+#define tr_error(...)
+#endif // MBED_CONF_MBED_TRACE_ENABLE


In the long run, I personally think we should avoid using mbed-trace altogether in platform, drivers and targets which are bare-minimal parts of mbed-os available to the bare metal profile by default.

mbed trace is fundamental enough that surely it should be in platform, or at least be included as a component by default, in which case it would provide its own dummy definitions.

It doesn't make sense to start scattering local dummy definitions everywhere,

Tracing should be no-op if disabled but available for everyone (part of platform).

@0xc0170 mbed_trace.h already contains dummy definitions (no-op), this is correct. The mbed-trace library isn't available to bare metal (unless the user adds it to mbed_app.json), and #include "mbed-trace/mbed_trace.h" fails.

One option is to add mbed-trace to

mbed-os/platform/bare_metal/mbed_lib.json

Lines 1 to 4 in 0db72d0

{

"name": "bare-metal",

"requires": ["platform", "drivers", "rtos-api"]

}

@kjbracey-arm said trace is fundamental enough, so shall we have it by default for bare metal? @evedon

By default tracing is disabled and so there is no memory cost for it. I agree that it is fundamental enough to be included by default in bare metal.

You can add a dummy "present" option to the mbed_lib.json for nanostack-libservice, then check its macro (MBED_CONF_NANOSTACK_LIBSERVICE_PRESENT). That's how we detect RTOS or RTOS API presence.

Then fea-ipv6 = truecould only take effect if that is turned on too.

Or, even more fancy, change the default for fea-ipv6 to null, which is interpreted as "on if libservice present". If the user explicitly sets it to true, then you can give an error telling them to add libservice if it's not present.

I can add that config to mbed-client-libservice (part of https://github.com/ARMmbed/nanostack-libservice), but this needs to be communicated with the client team who fetch libraries from standalone repos sometimes. Maybe the change needs to be "upstreamed".

Or, even more fancy, change the default for fea-ipv6 to null, which is interpreted as "on if libservice present".

It is already null 😉

Okay, yes, there are a few wrinkles there. The header defaults to IPv6 on if the MBED_CONF_MBED_TRACE_FEA_IPV6 is unset, which it is for null in Mbed OS, or if not using Mbed OS config.

You could distinguish that further by TARGET_LIKE_MBED - if that's defined, then check the MBED_CONF_NANOSTACK_LIBSERVICE_PRESENT, else assume on.

Still, could have a version mismatch where they get a new mbed trace from upstream but use it with old Mbed OS. Not sure if that's a possibility.

In non-Mbed OS builds, libservice is always there - it's Mbed OS that's being the odd thing out by potentially having mbed trace without libservice. Would it be the end of the world to add the dependency and add 10(?) more small source files to the bare build? Still shouldn't add code.

It is already null

Hmph. There's a bad habit of making boolean options be null/true, when they should be false/true. null should only be used for "none" (which would normally be in place of a number or string), or "default" - which could apply to booleans, but is only worth doing if it's some sort of "dynamic" setting, not just a constant true or false.

I'm going to make a PR with the minimum amount of check, and ping the client team.

Hmph. There's a bad habit of making boolean options be null/true, when they should be false/true. null should only be used for "none" (which would normally be in place of a number or string), or "default" - which could apply to booleans, but is only worth doing if it's some sort of "dynamic" setting, not just a constant true or false.

In fact, the code respects true/false set by users, and treats null as true otherwise. So now it's super easy for us to add the check for nanostack-libservice...

#13649 created

ciarmcom · 2020-09-15T16:30:29Z

@LDong-Arm, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

0xc0170

Shouldn't we fix mbed-trace rather ? As it is in platform, it is available in baremetal or not?

jeromecoutant · 2020-09-16T06:58:04Z

I'm slightly surprised we never caught this issue before. Unless I missed something...

I agree, I am starting full non regression tests using baremetal for each mbed-os-6 label without any issues...

jeromecoutant · 2020-09-16T07:07:30Z

I agree, I am starting full non regression tests using baremetal for each mbed-os-6 label without any issues...

mbed-trace is part of baremetal test config:

https://github.com/ARMmbed/mbed-os/blob/master/TESTS/configs/baremetal.json#L16

LDong-Arm · 2020-09-16T08:40:29Z

mbed-trace is part of baremetal test config:

Yes, the test config specifies mbed-trace to force it to be available to tests. But a general application may not force that, and targets shouldn't fail to compile. The compilation error can be reproduced with mbed-os-example-blinky-baremetal for DISCO_L475VG_IOT01A.

jeromecoutant · 2020-09-21T09:32:25Z

Shouldn't we fix mbed-trace rather ?

I agree. This PR should be closed

LDong-Arm · 2020-09-21T10:15:19Z

@jeromecoutant @0xc0170 As I said in my inline comment, the fix in mbed-trace is to disable ipv6 printing by default which depends on a network library. But that will break some existing applications. I wonder what we should do. @evedon

LDong-Arm · 2020-09-21T14:16:06Z

Replaced by #13649

TARGET_STM/qspi_api.c: fix build error when bare metal provides no mb…

a4f895a

…ed-trace

LDong-Arm commented Sep 15, 2020

View reviewed changes

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Sep 15, 2020

ciarmcom requested a review from a team September 15, 2020 16:30

ciarmcom added the needs: review label Sep 15, 2020

0xc0170 reviewed Sep 16, 2020

View reviewed changes

LDong-Arm mentioned this pull request Sep 21, 2020

Some Mbed OS 6 targets don't compile with Baremetal profile #13099

Closed

LDong-Arm mentioned this pull request Sep 21, 2020

Make mbed-trace available to bare metal #13649

Merged

LDong-Arm closed this Sep 21, 2020

mergify bot removed needs: review release-type: patch Indentifies a PR as containing just a patch labels Sep 21, 2020

	{
	"name": "bare-metal",
	"requires": ["platform", "drivers", "rtos-api"]
	}

TARGET_STM/qspi_api.c: fix build error on bare metal #13617

TARGET_STM/qspi_api.c: fix build error on bare metal #13617

Uh oh!

Conversation

LDong-Arm commented Sep 15, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

LDong-Arm commented Sep 15, 2020

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!

kjbracey Sep 21, 2020 • 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!

LDong-Arm Sep 21, 2020 • 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!

ciarmcom commented Sep 15, 2020

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

jeromecoutant commented Sep 16, 2020

Uh oh!

jeromecoutant commented Sep 16, 2020

Uh oh!

LDong-Arm commented Sep 16, 2020

Uh oh!

jeromecoutant commented Sep 21, 2020

Uh oh!

LDong-Arm commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LDong-Arm commented Sep 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kjbracey Sep 21, 2020 •

edited

Loading

LDong-Arm Sep 21, 2020 •

edited

Loading

LDong-Arm commented Sep 21, 2020 •

edited

Loading