STM32L4: Fix the UART RX & TX data reg bitmasks #12341

fkjagodzinski · 2020-01-30T16:52:45Z

Summary of changes

Update the STM HAL to pass the tests that make use of the FPGA CI test shield:

Fix the UART RX & TX data reg bitmasks on STM32L4
~~tests-mbed_hal_fpga_ci_test_shield-uart for:~~ <- this is fixed by Add STDIO UART as restricted for FPGA testing for all targets and support for restricting GPIO #12379
- ~~DISCO_L475VG_IOT01A,~~
- ~~NUCLEO_F303RE,~~
~~tests-mbed_hal_fpga_ci_test_shield-gpio for:~~
- ~~DISCO_L475VG_IOT01A~~ <- this is moved to DISCO_L475VG_IOT01A: Add a list of restricted GPIO pins for testing #12380

Impact of changes

Migration actions required

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/team-st-mcd, @jamesbeyond, @mprse, @maciejbocianski

ciarmcom · 2020-01-30T18:00:42Z

@fkjagodzinski, thank you for your changes.
@maciejbocianski @jamesbeyond @ARMmbed/mbed-os-maintainers please review.

LMESTM

I think that only the 3rd patch (STM32L4: Fix the UART RX & TX data reg bitmasks) looks good to me and should be pushed in a separate dedicated pull requests.
I would prefer that you look for better alternatives to the 2 other patches.

LMESTM · 2020-01-31T12:23:19Z

.../TARGET_STM/TARGET_STM32L4/TARGET_STM32L475xG/TARGET_DISCO_L475VG_IOT01A/PeripheralPinMaps.h

@@ -52,6 +52,94 @@
 #include "PinNamesTypes.h"
 #include <mstd_cstddef>

+//*** GPIO ***
+
+MSTD_CONSTEXPR_OBJ_11 PinMap PinMap_GPIO[] = {


If this is only for testing I would that this code only exists for tests ... as of now this table seems to be unconditionally compiled. Also we want to be able to test any platform with FPGA test shield and I would like to avoid that we have to add such a dummy table to all our Targets ... all in all I am not in favor of this change, I hope that can be a better alternative

I've just confirmed that the PinMap_GPIO symbol is discarded at link time if not used (by building a blinky example). When building tests-mbed_hal_fpga_ci_test_shield-gpio the elf does contain it, as expected.
There is no need to add this to other targets if they pass the FPGA GPIO test. A default, weak implementation is used (https://github.com/Armmbed/mbed-os/blob/master/hal/mbed_gpio.c#L77). We're overriding the PinMap_GPIO only if there is a HW limitation (like a pull-up) for any pins.

LMESTM · 2020-01-31T12:26:34Z

targets/TARGET_STM/pinmap.cpp

+/**
+ * List of peripherals excluded from testing
+ */
+const PeripheralList *pinmap_restricted_peripherals()


Same here: I'd prefer not to impact the generic source code for tests purposed.
There is already a generic definition of the stdio_uart object that is used at platform level:
platform/source/mbed_retarget.cpp:extern serial_t stdio_uart;
Couldn't this be used for excluding stdio_uart from the test suite without changing the target code ?

Moreover there is no other cpp files in the HAL, I feel like we're losing some consistency here

I understand you don't like the .c -> .cpp change. The use of pinmap_peripheral() was my idea for covering a wider range of targets than those that define STDIO_UART peripheral.

I think that skipping the tests for

mbed-os/platform/source/mbed_retarget.cpp

Line 148 in 236c336

extern serial_t stdio_uart;

is a good alternative. In general we use the pinmap_restricted_peripherals() for this purpose in FPGA tests.

@mprse will pick this PR next week. I'll let him decide how to update this.

serial_t stdio_uart can not be used to initialize peripherals[] because it is not constant .

LMESTM · 2020-01-31T12:30:11Z

@fkjagodzinski thanks for your proposed changes. I have made comments as I think this proposal should better be split into several separate ones so that the UART fix can be accepted while other changes can be further discussed ...

jeromecoutant · 2020-02-03T08:19:11Z

targets/TARGET_STM/pinmap.cpp

+const PeripheralList *pinmap_restricted_peripherals()
+{
+#if DEVICE_SERIAL
+    static const int stdio_uart = pinmap_peripheral(STDIO_UART_TX, PinMap_UART_TX);


Hi
It should be easier to comment the lines in the PinMap table ?

bulislaw

One thing to consider except whether the linker can deal with the change is user perception. I think explicitly calling it as a test object/feature avoid some confusion and doesn't tempt people into using it.

jeromecoutant · 2020-02-04T14:24:58Z

I think that only the 3rd patch (STM32L4: Fix the UART RX & TX data reg bitmasks) looks good to me and should be pushed in a separate dedicated pull requests.
I would prefer that you look for better alternatives to the 2 other patches.

@fkjagodzinski
Is it ok to keep only the patch for uart reg bitmask in this PR ?

mprse · 2020-02-05T12:56:12Z

I discussed the issues reported here with @maciejbocianski and we have the following thoughts.

UART restricted peripherals

The proposition is to remove related changes from this PR and add restricted STDIO UART peripheral for all targets here:

mbed-os/hal/mbed_pinmap_default.c

Lines 80 to 88 in b140fd0

    
           //*** Default restricted peripherals *** 
        
           MBED_WEAK const PeripheralList *pinmap_restricted_peripherals() 
        
           { 
        
               static const PeripheralList peripheral_list = { 
        
                   0, 
        
                   0 
        
               }; 
        
               return &peripheral_list; 
        
           }

Substantiation for this is that the STDIO UART peripheral is used by Mbed, so it should never be tested.

While looking into this we also found a potential problem. Currently, we have a one pinmap_restricted_peripherals() function for all interfaces (UART, I2C, SPI, etc.). The problem can be encountered if different interfaces have the same peripheral ids (e.g. UART_ 0 = 0, SPI_0 = 0). In this case, if UART_ 0 is on the restricted list, then SPI tests will be also skipped for SPI_0.
The good news is that usually, the peripheral ids are the base addresses of the peripheral's register set, but we can't rely on this. It is also good that pinmap_restricted_peripherals() at this moment is only required by STDIO UART (Nuvoton and STM).

The proposition is to use different functions for each interface: pinmap_uart_restricted_peripherals(), pinmap_spi_restricted_peripherals(), etc. This will also require to update the test utility module to check restrictions per interface:

mbed-os/components/testing/COMPONENT_FPGA_CI_TEST_SHIELD/test_utils.h

Lines 118 to 121 in b140fd0

    
           if (pinmap_list_has_peripheral(pinmap_restricted_peripherals(), port.peripheral)) { 
        
               utest_printf("Skipping %s peripheral %i with pin %s (%i)\r\n", pin_type, 
        
                            port.peripheral, FormFactorType::pin_to_string(port.pins[i]), port.pins[i]); 
        
               continue;

@jeromecoutant @LMESTM @bulislaw @jamesbeyond
Please let me know if this approach is acceptable.

@jamesbeyond
And if I should proceed with the proposed changes.

Dummy GPIO pin-map

I understand that you don't like dummy pin-map which is required for testing only in your STM files, but this is our common approach in this case. This is required because of target limitations, so we need to add such a table per target. Unfortunately, I can't see a better place to put this, then together with other pin-map tables. What I can suggest is that we can add a comment that this is the pin-map table for testing purposes only. Additionally, this table contains pieces of information that can be valuable for the user (pins that have limited GPIO functionalities are listed with the note).

@jeromecoutant @LMESTM @bulislaw @jamesbeyond
Please let me know what's your thoughts.

mprse · 2020-02-06T11:52:02Z

Provided the following changes (proposed above):

dropped commit which was adding pinmap_restricted_peripherals() implementation for STM,
changed weak implementation of pinmap_restricted_peripherals() in order to restrict STDIO uart peripheral for all targets and renamed to pinmap_uart_restricted_peripherals() PR Add STDIO UART as restricted for FPGA testing for all targets and support for restricting GPIO #12379.
modified utility functions to support skipping UART peripherals only (only this is required at the moment; support for other interfaces can be added later) PR Add STDIO UART as restricted for FPGA testing for all targets and support for restricting GPIO #12379,
added a comment to inform that GPIO pinmap is added only for FPGA testing purposes - PR DISCO_L475VG_IOT01A: Add a list of restricted GPIO pins for testing #12380.

Please review.

jeromecoutant · 2020-02-06T12:14:50Z

Please make 1 Pull Request for FPGA part, 1 for NUVUTON, 1 for ST, 1 for..... ?

The existing logic was insufficient to properly handle odd and even parity setting, e.g. serial_getc() returned 9-bit data for 8O1 transmission format.

mprse · 2020-02-06T13:12:06Z

Provided the following changes:

dropped all commits except patch for UART reg bitmask in this PR,
change the weak implementation of pinmap_restricted_peripherals() in order to restrict STDIO uart peripheral for all targets - moved to PR Add STDIO UART as restricted for FPGA testing for all targets and support for restricting GPIO #12379.
add GPIO pinmap for DISCO_L475VG_IOT01A only for FPGA testing purposes - moved to PR DISCO_L475VG_IOT01A: Add a list of restricted GPIO pins for testing #12380.

mprse · 2020-02-06T13:18:59Z

@0xc0170 Can you rename the PR: STM32L4: Fix the UART RX & TX data reg bitmasks. ?

LMESTM

Thx for this fix !

jeromecoutant · 2020-02-06T14:50:35Z

I applied the patch to other STM32 families: fkjagodzinski#4

jamesbeyond · 2020-02-06T15:24:39Z

@0xc0170 Can you rename the PR: STM32L4: Fix the UART RX & TX data reg bitmasks. ?

Done

0xc0170 · 2020-02-07T15:22:07Z

CI started

jeromecoutant · 2020-02-07T15:26:00Z

I applied the patch to other STM32 families: fkjagodzinski#4

I don't see it merged into that PR,
do you prefer a new pull request ?

Pull request has been modified.

jamesbeyond · 2020-02-07T16:25:27Z

I applied the patch to other STM32 families: fkjagodzinski#4

I don't see it merged into that PR,
do you prefer a new pull request ?

Hi @jeromecoutant, Filip is off this week, I just got your PR merged.

jamesbeyond

Looks good, Thanks for the update @jeromecoutant @mprse @fkjagodzinski

mbed-ci · 2020-02-07T17:38:35Z

Test run: SUCCESS

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

jamesbeyond · 2020-02-09T21:53:33Z

CI restarted due to merge of fkjagodzinski#4

mbed-ci · 2020-02-09T23:42:23Z

Test run: SUCCESS

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

fkjagodzinski · 2020-02-10T13:23:10Z

I applied the patch to other STM32 families: fkjagodzinski#4

Thank you @jeromecoutant 👍

jamesbeyond requested review from jeromecoutant and mprse January 30, 2020 17:42

ciarmcom requested review from jamesbeyond, maciejbocianski and a team January 30, 2020 18:00

ciarmcom added the needs: review label Jan 30, 2020

LMESTM suggested changes Jan 31, 2020

View reviewed changes

jeromecoutant reviewed Feb 3, 2020

View reviewed changes

bulislaw reviewed Feb 3, 2020

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Feb 4, 2020

mprse force-pushed the fix-stm-hal_fpga branch from 9d45c00 to 707f1da Compare February 6, 2020 11:31

STM32L4: Fix the UART RX & TX data reg bitmasks

ae635d5

The existing logic was insufficient to properly handle odd and even parity setting, e.g. serial_getc() returned 9-bit data for 8O1 transmission format.

mprse force-pushed the fix-stm-hal_fpga branch from 707f1da to ae635d5 Compare February 6, 2020 13:08

LMESTM approved these changes Feb 6, 2020

View reviewed changes

jamesbeyond changed the title ~~STM: Fix UART & GPIO HAL to pass FPGA CI test shield tests~~ STM32L4: Fix the UART RX & TX data reg bitmasks Feb 6, 2020

jamesbeyond previously approved these changes Feb 6, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Feb 6, 2020

STM32: Fix the UART RX & TX data reg bitmasks

2368a07

jamesbeyond approved these changes Feb 7, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Feb 9, 2020

0xc0170 added the release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 label Feb 10, 2020

0xc0170 merged commit 7fd5119 into ARMmbed:master Feb 10, 2020

mergify bot removed the ready for merge label Feb 10, 2020

fkjagodzinski deleted the fix-stm-hal_fpga branch February 10, 2020 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STM32L4: Fix the UART RX & TX data reg bitmasks #12341

STM32L4: Fix the UART RX & TX data reg bitmasks #12341

fkjagodzinski commented Jan 30, 2020 •

edited by jamesbeyond

Loading

ciarmcom commented Jan 30, 2020

LMESTM left a comment

LMESTM Jan 31, 2020

fkjagodzinski Jan 31, 2020

LMESTM Jan 31, 2020

fkjagodzinski Jan 31, 2020

mprse Feb 5, 2020 •

edited

Loading

LMESTM commented Jan 31, 2020

jeromecoutant Feb 3, 2020

bulislaw left a comment

jeromecoutant commented Feb 4, 2020

mprse commented Feb 5, 2020

mprse commented Feb 6, 2020 •

edited

Loading

jeromecoutant commented Feb 6, 2020

mprse commented Feb 6, 2020

mprse commented Feb 6, 2020

LMESTM left a comment

jeromecoutant commented Feb 6, 2020

jamesbeyond commented Feb 6, 2020

0xc0170 commented Feb 7, 2020

jeromecoutant commented Feb 7, 2020

jamesbeyond commented Feb 7, 2020

jamesbeyond left a comment

mbed-ci commented Feb 7, 2020

jamesbeyond commented Feb 9, 2020

mbed-ci commented Feb 9, 2020

fkjagodzinski commented Feb 10, 2020

STM32L4: Fix the UART RX & TX data reg bitmasks #12341

STM32L4: Fix the UART RX & TX data reg bitmasks #12341

Conversation

fkjagodzinski commented Jan 30, 2020 • edited by jamesbeyond Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Jan 30, 2020

LMESTM left a comment

Choose a reason for hiding this comment

LMESTM Jan 31, 2020

Choose a reason for hiding this comment

fkjagodzinski Jan 31, 2020

Choose a reason for hiding this comment

LMESTM Jan 31, 2020

Choose a reason for hiding this comment

fkjagodzinski Jan 31, 2020

Choose a reason for hiding this comment

mprse Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

LMESTM commented Jan 31, 2020

jeromecoutant Feb 3, 2020

Choose a reason for hiding this comment

bulislaw left a comment

Choose a reason for hiding this comment

jeromecoutant commented Feb 4, 2020

mprse commented Feb 5, 2020

mprse commented Feb 6, 2020 • edited Loading

jeromecoutant commented Feb 6, 2020

mprse commented Feb 6, 2020

mprse commented Feb 6, 2020

LMESTM left a comment

Choose a reason for hiding this comment

jeromecoutant commented Feb 6, 2020

jamesbeyond commented Feb 6, 2020

0xc0170 commented Feb 7, 2020

jeromecoutant commented Feb 7, 2020

jamesbeyond commented Feb 7, 2020

jamesbeyond left a comment

Choose a reason for hiding this comment

mbed-ci commented Feb 7, 2020

Test run: SUCCESS

jamesbeyond commented Feb 9, 2020

mbed-ci commented Feb 9, 2020

Test run: SUCCESS

fkjagodzinski commented Feb 10, 2020

fkjagodzinski commented Jan 30, 2020 •

edited by jamesbeyond

Loading

mprse Feb 5, 2020 •

edited

Loading

mprse commented Feb 6, 2020 •

edited

Loading