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

STM32 STDIO pins number are now configurable #5795

Merged
merged 10 commits into from
Jan 16, 2018

Conversation

jeromecoutant
Copy link
Collaborator

Description

With that pull request, default configuration is not changed.

There are 2 goals for this PR:

  • Help user when he wrongly defines an alternate UART pins from the same UART instance as the one configured for STDIO
    Now the failure becomes blocking, and an error message is printed.

  • Help user to define his own STDIO pins (for a custom board for ex or if ST-Link is removed), only a json configuration update is needed.
    A good example could be the UBLOX board:

    "UBLOX_EVK_ODIN_W2": {
    "config": {
    "stdio_uart_tx": {
    "help": "Value: D8(default) or D1",
    "value": "D8"
    },
    "stdio_uart_rx": {
    "help": "Value: D2(default) or D0",
    "value": "D2"
    }
    }

@andreaslarssonublox

Status

READY

- STDIO_UART define is no more used
- configuring a new serial with the same UART as STDIO is no more allowed
@@ -1801,57 +1801,42 @@
"supported_form_factors": ["ARDUINO"],
"release_versions": ["5"],
"config": {
"usb_tx": {
"stdio_uart_tx": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the old name is used, this breaks it. Shouldn't this be done backward compatible, and then cleaned up for the minor where it can be documented how to update the application?

This change could be as is, but the target code should handle both cases (thus target override in the app would work if it overrides usb_tx there)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree.
I will update code.

@jeromecoutant
Copy link
Collaborator Author

@0xc0170
Change done, see last commit 9c6e7c0

"macro_name": "STDIO_UART"
}
}
"release_versions": ["5"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad rebase ? these pins should be redefined here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as there is no "user choice" for this target, the default configuration in the PinName.h file should be sufficient ?
No need to overload the json file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering how this is different than from Odin target (line 1818 changes it to a new name) but here you remove it here, thus I asked.

Not clear the line 1831 removal of stdio_uart for odin also?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, STDIO_UART is not used any more in serial_api.c file

@jeromecoutant
Copy link
Collaborator Author

Here is a wiki page to help user : https://os.mbed.com/teams/ST/wiki/STDIO

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 12, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jan 12, 2018

Build : SUCCESS

Build number : 855
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5795/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci
Copy link

mbed-ci commented Jan 12, 2018

@mbed-ci
Copy link

mbed-ci commented Jan 12, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 15, 2018

@jeromecoutant Please can you review the failures

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 15, 2018

/morph test

@mbed-ci
Copy link

mbed-ci commented Jan 15, 2018

@cmonr cmonr merged commit f01fbde into ARMmbed:master Jan 16, 2018
@jeromecoutant jeromecoutant deleted the PR_USER_DEFINED_STDIO branch January 17, 2018 15:52
jeromecoutant added a commit to jeromecoutant/mbed that referenced this pull request Jan 26, 2018
ARMmbed#5795 patches are missing for these 2 targets
STDIO_UART_TX and STDIO_UART_RX can be now user defined
@atoy40
Copy link

atoy40 commented Feb 6, 2018

@jeromecoutant I'm testing 5.7.4 and this feature with a L073RZ board and there is an issue.
As stdio_uart_tx/rx are not defined into the target file, you cannot use in your config :

"target_overrides": {
        "NUCLEO_L073RZ": {
             "target.stdio_uart_tx": "tx pin name",
             "target.stdio_uart_rx": "rx pin name",
        }
    }

It is ignored and does not generate macro into mbed_config.h.
As you've used MBED_CONF_TARGET_STDIO_UART_* as macro name in PinNames.h, you cannot write :

"config": {
          "stdio_uart_tx": {
              "value": "tx pin name"
          },
          "stdio_uart_rx": {
              "value": "rx pin name"
          }
      }

because config variables are prefixed with MBED_CONF_APP_

The workarround is to force macro name :

"config": {
        "stdio_uart_tx": {
            "value": "tx pin name",
            "macro_name": "MBED_CONF_TARGET_STDIO_UART_TX"
        },
        "stdio_uart_rx": {
            "value": "rx pin name",
            "macro_name": "MBED_CONF_TARGET_STDIO_UART_RX"
        }
    },

but is it expected ? I imagine the first code sample is the one to use ?

thanks
Anthony.

@jeromecoutant
Copy link
Collaborator Author

Hi

As indicated in https://os.mbed.com/teams/ST/wiki/STDIO

Here is how I checked it with a mbed_app.json :

{
"config": {
"stdio_uart_tx": {
"value": "PA_2"
},
"stdio_uart_rx": {
"value": "PA_3"
}
}
}

It is true it is not specific to NUCLEO_L073RZ in this case...

@atoy40
Copy link

atoy40 commented Feb 6, 2018

@jeromecoutant as I said, if you use that, your two macros will be named : MBED_CONF_APP_XXX and not MBED_CONF_TARGET_XXX as it is expected in the code (PinNames.h).

@jeromecoutant
Copy link
Collaborator Author

Mmmm

Maybe I should add an empty config in the FAMILY_STM32 in targets.json ?

    "config": {
        "lse_available": { ... }
        "stdio_uart_tx": {
            "help": "xxx"
        }
        "stdio_uart_rx": {
            "help": "xxx"
        }
    },

Let's try

@atoy40
Copy link

atoy40 commented Feb 6, 2018

Yes this is what you have to do to allow target_overrides. And why do not put default board value in the target file ? instead of putting it into PinNames.h ?

@jeromecoutant
Copy link
Collaborator Author

why do not put default board value in the target file ?

Because I don't want to make this huge rework... :-)
I am going to send a PR with the above proposition

adbridge pushed a commit that referenced this pull request Feb 9, 2018
#5795 patches are missing for these 2 targets
STDIO_UART_TX and STDIO_UART_RX can be now user defined
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants