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

Fix failure of building non-RTOS for GR-PEACH, GR-LYCHEE and VK-RZ/A1H #11816

Merged
merged 3 commits into from Nov 7, 2019

Conversation

@d-kato
Copy link
Contributor

d-kato commented Nov 6, 2019

Description (required)

Summary of change (What the change is for and why)

Fix for issue ARMmbed/mbed-os-example-blinky-baremetal#20
I confirmed that mbed-os-example-blinky-baremetal can be built and blinky works properly.
The following is the GR-PEACH build log. GR-LYCHEE and VK-RZ / A1H will build successfully as well.

Building project mbed-os-example-blinky-baremetal (RZ_A1H, GCC_ARM)
Scan: mbed-os-example-blinky-baremetal
Link: mbed-os-example-blinky-baremetal
Elf2Bin: mbed-os-example-blinky-baremetal
| Module           |     .text |    .data |     .bss |
|------------------|-----------|----------|----------|
| [fill]           |    12(+0) |    6(+0) |   25(+0) |
| [lib]\c.a        | 16348(+0) | 2472(+0) |   89(+0) |
| [lib]\gcc.a      |  1008(+0) |    0(+0) |    0(+0) |
| [lib]\misc       |   252(+0) |    4(+0) |   28(+0) |
| [misc]           |     0(+0) |    0(+0) |    0(+0) |
| main.o           |   104(+0) |    0(+0) |   24(+0) |
| mbed-os\cmsis    |   896(+0) |    0(+0) | 4084(+0) |
| mbed-os\drivers  |   284(+0) |    0(+0) |    0(+0) |
| mbed-os\hal      |  2356(+0) |    8(+0) |  131(+0) |
| mbed-os\platform |  4528(+0) |  260(+0) |  188(+0) |
| mbed-os\targets  |  6524(+0) |    2(+0) |  723(+0) |
| Subtotals        | 32312(+0) | 2752(+0) | 5292(+0) |
Total Static RAM memory (data + bss): 8044(+0) bytes
Total Flash memory (text + data): 35064(+0) bytes

Image: .\BUILD\RZ_A1H\GCC_ARM\mbed-os-example-blinky-baremetal.bin

There was no problem when using RTOS (removed "requires": ["bare-metal"] in mbed_app.json).

Building project mbed-os-example-blinky-baremetal (RZ_A1H, GCC_ARM)
Scan: mbed-os-example-blinky-baremetal
Link: mbed-os-example-blinky-baremetal
Elf2Bin: mbed-os-example-blinky-baremetal
| Module           |     .text |    .data |      .bss |
|------------------|-----------|----------|-----------|
| [fill]           |    24(+0) |   13(+0) |    28(+0) |
| [lib]\c.a        | 16348(+0) | 2472(+0) |    89(+0) |
| [lib]\gcc.a      |  1008(+0) |    0(+0) |     0(+0) |
| [lib]\misc       |   252(+0) |    4(+0) |    28(+0) |
| [misc]           |     0(+0) |    0(+0) |     0(+0) |
| main.o           |   104(+0) |    0(+0) |    24(+0) |
| mbed-os\cmsis    |  1292(+0) |    0(+0) |  4084(+0) |
| mbed-os\drivers  |   128(+0) |    0(+0) |     0(+0) |
| mbed-os\hal      |  2040(+0) |    4(+0) |    67(+0) |
| mbed-os\platform |  3624(+0) |  260(+0) |   136(+0) |
| mbed-os\rtos     | 11120(+0) |  173(+0) |  5972(+0) |
| mbed-os\targets  |  6420(+0) |    2(+0) |   720(+0) |
| Subtotals        | 42360(+0) | 2928(+0) | 11148(+0) |
Total Static RAM memory (data + bss): 14076(+0) bytes
Total Flash memory (text + data): 45288(+0) bytes

Image: .\BUILD\RZ_A1H\GCC_ARM\mbed-os-example-blinky-baremetal.bin
Documentation (Details of any document updates required)

Pull request type (required)

[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 (required)

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

Reviewers (optional)

@toyowata

@toyowata

This comment has been minimized.

Copy link
Contributor

toyowata commented Nov 6, 2019

@d-kato

Thank you very much for the patch. I tested it and confimred that LED1 blinks as expect timming.
I added printf message in the source code for additional test, and got strage output.

  • Test code with printf
#include "mbed.h"

#define WAIT_TIME 500 //msec

DigitalOut led1(LED1);

int main()
{
    int cnt = 0;
    printf("hello\n");
    while (true)
    {
        printf("cnt = %d\n", cnt++);
        led1 = !led1;
        thread_sleep_for(WAIT_TIME);
    }
}
  • Log output (baremetal)
ÿhello
cnt = R4`¤OÿLcnt = &¡,
                      ¤Oÿ&¡,
                            ¤O¡,
                                ¤OÿMcnt = ¡,
                                            ¤O?,
                                                ¤OR4`¤OÿNcnt = 1R4`¤O1ÿLcnt = 1&¡,
  ¤O1ÿ&¡,
         ¤O1


  • Without baremetal (RTOS version) works just fine
hello
cnt = 0
cnt = 1
cnt = 2
cnt = 3
cnt = 4
cnt = 5
cnt = 6
cnt = 7
cnt = 8
cnt = 9
cnt = 10
cnt = 11

Can you check?

@ciarmcom ciarmcom requested review from toyowata and ARMmbed/mbed-os-maintainers Nov 6, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Nov 6, 2019

@d-kato, thank you for your changes.
@toyowata @ARMmbed/mbed-os-maintainers please review.

@d-kato

This comment has been minimized.

Copy link
Contributor Author

d-kato commented Nov 6, 2019

@toyowata I will check about this.

@d-kato

This comment has been minimized.

Copy link
Contributor Author

d-kato commented Nov 6, 2019

@toyowata It was because hal_deepsleep was called before printf transmission was completed.
printf calls the following function:

ssize_t DirectSerial::write(const void *buffer, size_t size)
{
const unsigned char *buf = static_cast<const unsigned char *>(buffer);
for (size_t i = 0; i < size; i++) {
serial_putc(&stdio_uart, buf[i]);
}
return size;
}

serial_putc was changed to wait for transmission completion.

  • Log of baremetal (non-RTOS version)
hello
cnt = 0
cnt = 1
cnt = 2
cnt = 3
  • Log of without baremetal (RTOS version)
hello
cnt = 0
cnt = 1
cnt = 2
cnt = 3
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Nov 6, 2019

get_a9_tick change is fine, but the serial shouldn't be waiting for completion in serial_putc itself - it's bad for performance and interrupt latency.

It's better to deal with that issue in hal_sleep - see discussion on #11401

Copy link
Member

0xc0170 left a comment

As requested above, serial change

@0xc0170 0xc0170 added needs: work and removed needs: review labels Nov 6, 2019
@d-kato

This comment has been minimized.

Copy link
Contributor Author

d-kato commented Nov 7, 2019

@toyowata @kjbracey-arm @0xc0170 Thank you for the review. I moved the wait for transmission completion from serial_putc to hal_deepsleep. I confirmed that printf was displayed correctly.

@0xc0170 0xc0170 self-requested a review Nov 7, 2019
@0xc0170
0xc0170 approved these changes Nov 7, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 7, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Nov 7, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Nov 7, 2019
@0xc0170 0xc0170 merged commit e89ce4f into ARMmbed:master Nov 7, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8684 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8420B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170 0xc0170 removed the ready for merge label Nov 7, 2019
for (int uart = 0; uart < SCIF_COUNT; uart++) {
if ((wk_CPGSTBCR4 & (1 << (7 - uart))) == 0) { // Is the power turned on?
if ((SCIF[uart]->SCSCR & 0x00A0) == 0x00A0) { // Is transmission enabled? (TE = 1, TIE = 1)
while ((SCIF[uart]->SCFSR & 0x0040) == 0); // Waits for the transmission to complete (TEND = 1)

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Nov 7, 2019

Contributor

From my thoughts on #11401, I think it's preferable to have hal_deepsleep return immediately if it sees any serial port is busy - that way we can respond if some interrupt occurs while waiting for the port to drain. (The system will repeatedly call hal_deepsleep, so the loop occurs at a higher level).

This form might be bad for interrupt latency if the baud rate is low enough for this loop to take significant time.

Something to consider for a future improvement. Conceivably some HAL test might be upset by that "return immediately" behaviour, even though it would make sense in the real sleep code.

This is a theoretical point of view, though - I've not seen any real problem with this form. I guess if we're going into deep sleep we can assume we're not at a point where interrupt latency really matters...

This comment has been minimized.

Copy link
@d-kato

d-kato Nov 8, 2019

Author Contributor

Thank you for your feedback. I'll make another pull request for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.