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

d-kato · 2019-11-06T06:25:21Z

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 · 2019-11-06T07:23:23Z

@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 · 2019-11-06T08:00:30Z

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

d-kato · 2019-11-06T08:05:36Z

@toyowata I will check about this.

d-kato · 2019-11-06T12:13:45Z

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

mbed-os/platform/source/mbed_retarget.cpp

Lines 181 to 188 in 3254ec3

    
           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 · 2019-11-06T13:17:45Z

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

0xc0170

As requested above, serial change

d-kato · 2019-11-07T03:33:36Z

@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 · 2019-11-07T08:42:44Z

CI started

mbed-ci · 2019-11-07T10:10:14Z

Test run: SUCCESS

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

kjbracey · 2019-11-07T10:26:43Z

targets/TARGET_RENESAS/TARGET_RZ_A1XX/sleep.c

+    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)


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...

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

Moved waiting UART transmission completion to the out of critical section. This is issued by the following pull request. ARMmbed#11816

Fix missing mbed_get_a9_tick_irq in non-RTOS build

076bbe9

ciarmcom requested review from toyowata and a team November 6, 2019 08:00

ciarmcom added the needs: review label Nov 6, 2019

serial_putc waits for the transmission to complete

86f648d

0xc0170 requested changes Nov 6, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Nov 6, 2019

Moved transmission completion wait to hal_deepsleep

4fbb87d

toyowata approved these changes Nov 7, 2019

View reviewed changes

0xc0170 self-requested a review November 7, 2019 08:41

0xc0170 approved these changes Nov 7, 2019

View reviewed changes

0xc0170 added needs: CI release-version: 5.14.2 and removed needs: work labels Nov 7, 2019

0xc0170 added ready for merge and removed needs: CI labels Nov 7, 2019

0xc0170 merged commit e89ce4f into ARMmbed:master Nov 7, 2019

0xc0170 removed the ready for merge label Nov 7, 2019

kjbracey reviewed Nov 7, 2019

View reviewed changes

RyoheiHagimoto mentioned this pull request Oct 9, 2020

Renesas: fix timing to wait UART completion in deep sleep function #13747

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

d-kato commented Nov 6, 2019

Uh oh!

toyowata commented Nov 6, 2019

Uh oh!

ciarmcom commented Nov 6, 2019

Uh oh!

d-kato commented Nov 6, 2019

Uh oh!

d-kato commented Nov 6, 2019

Uh oh!

kjbracey commented Nov 6, 2019

Uh oh!

0xc0170 left a comment

Uh oh!

d-kato commented Nov 7, 2019

Uh oh!

0xc0170 commented Nov 7, 2019

Uh oh!

mbed-ci commented Nov 7, 2019

Uh oh!

kjbracey Nov 7, 2019

Uh oh!

d-kato Nov 8, 2019

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

d-kato commented Nov 6, 2019

Description (required)

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

Documentation (Details of any document updates required)

Pull request type (required)

Test results (required)

Reviewers (optional)

Uh oh!

toyowata commented Nov 6, 2019

Uh oh!

ciarmcom commented Nov 6, 2019

Uh oh!

d-kato commented Nov 6, 2019

Uh oh!

d-kato commented Nov 6, 2019

Uh oh!

kjbracey commented Nov 6, 2019

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

d-kato commented Nov 7, 2019

Uh oh!

0xc0170 commented Nov 7, 2019

Uh oh!

mbed-ci commented Nov 7, 2019

Test run: SUCCESS

Uh oh!

kjbracey Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

d-kato Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!