STM32H7: Don't configure GPIO port pinout for _c pins #14707

pennam · 2021-05-26T15:24:19Z

Summary of changes

On PORTENTA, but also on DISCO-H747I PA1 pin is used as ethernet refclock and PA1_C as analog input.

When ethernet connection is running instantiating an AnalogIn input on PA1_C will cause ethernet connection hang.

It's my understanding that _c pins are not mapped as all the other GPIO because they are analog only pins. Calling pinmap_pinout function on _c pins actually changes the configuration of the non _c pin.

pinmap_pinout(PA_0C, PinMap_ADC) changes the output configuration of PA_0;
pinmap_pinout(PA_1C, PinMap_ADC) changes the output configuration of PA_1 and so on...

Impact of changes

STM32H747xI STM32H7A3xIQ STM32H743xI

Migration actions required

Documentation

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

To reproduce the issue this program can be used, at the moment i've only tested it on PORTENTA

#include "mbed.h"
#include "EthernetInterface.h"
#include "TCPSocket.h"
 
EthernetInterface   eth;
TCPSocket           server;
TCPSocket           *client;
SocketAddress       clientAddress;

char str[128];

// Socket demo
int main()
{
    // Bring up the ethernet interface
    printf("Ethernet webserver example\n");
    eth.connect();

    // Show the network address
    SocketAddress a;
    eth.get_ip_address(&a);
    printf("IP address: %s\n", a.get_ip_address() ? a.get_ip_address() : "None");

    // TCP Socket server
    server.open(&eth);
    server.bind(80);
    server.listen(5);
 
    while (true) {
        client = server.accept();
        client->getpeername(&clientAddress);
        printf("accept %s:%d\n", clientAddress.get_ip_address(), clientAddress.get_port());
        
        client->send("HTTP/1.1 200 OK\r\n", 17);
        client->send("Content-Type: text/html\r\n", 25);
        client->send("Connection: close\r\n", 19);  // the connection will be closed after completion of the response
        client->send("Refresh: 1,10\r\n", 15);  // refresh the page automatically every 1 sec
        client->send("\r\n", 2);
        client->send("<!DOCTYPE HTML>\r\n", 17);
        client->send("<html>\r\n", 8);
        
        // output the value of analog input
        AnalogIn   a1(PA_1C);
        client->send("analog input 1 is \r\n", 20);
        sprintf(str,"%f <br />\r\n", a1.read());
        client->send(str, strlen(str));
        memset(str, 0, 128);

        client->send("</html>\r\n", 8);
        
        client->close();
    }
}

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

Reviewers

@facchinm @jeromecoutant

ciarmcom · 2021-05-26T15:30:19Z

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

jeromecoutant · 2021-05-26T16:05:29Z

targets/TARGET_STM/TARGET_STM32H7/analogin_device.c

        // Configure GPIO
+#if defined(ALTC)
+    if ((pin != PA_0C) && (pin != PA_1C) && (pin != PC_2C) && (pin != PC_3C))
+#endif


This will stop analog support of arduino Ax pins ?

https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32H7/TARGET_STM32H747xI/TARGET_DISCO_H747I/PinNames.h#L308-L311

No, analog Ax pins are working also after the change. On our board we are using PA_0C PA_1C PC_2C PC_3C PC2 PC3 PA4 and PA6 as analog input pins and they are all working. It's my understanding that there is no need (and it is in some way wrong) to call the pinmap_pinout functions for PX_XC because they cannot be configured as all the others GPIO. pinmap_pinout function is used to change GPIO port configuration (pull-up, pull-down, speed, input output...) but this pins don't have these options.

OK, it's working because in analogin, pinmap_pinout sets pin to analog and PullNone, which is the default pin state...

But I don't like the current correction :-)
I am thinking about a better patch in pinmap.c

Could you have a look on jeromecoutant@ca862ed

pennam · 2021-05-31T15:37:11Z

Hi @jeromecoutant thanks for your proposal. Creating configure_dualpad_switch() inside pinmap.c makes a lot of sense regardless my changes. I dind't have the chance to test it yet, but i don't think this jeromecoutant@ca862ed changes will fix the issue with the ethernet interface.
The point is here

mbed-os/targets/TARGET_STM/pinmap.c

Lines 70 to 75 in 8a0c3c7

    
           uint32_t port = STM_PORT(pin); 
        
           uint32_t ll_pin  = ll_pin_defines[STM_PIN(pin)]; 
        
           uint32_t ll_mode = 0; 
        
           // Enable GPIO clock 
        
           GPIO_TypeDef *const gpio = Set_GPIO_Clock(port);

calling pin_function(PA_1C, data)

gpio will point to port A address and ll_pin is 1 so it will reconfigure PA1 as analog input, breaking ethernet interface. There are 16 GPIO for each port from PX_0 to PX_15 so my idea is that PX_1C does not fit into these registers, and they don't have to be configured for _C pins. I will try to test it again tomorrow adding some debug prints to be more confident of the outcome.

jeromecoutant · 2021-06-01T10:06:21Z

OK! We agree that the pinmap.c implementation still misses some use case!
I have discussed with my STM32duino colleagues (from where I copied the patch...), they will propose an update as well!

I propose to keep this PR opened till a full and verified version is not merged.

jeromecoutant

Waiting for the correction in pinmap.c

ciarmcom · 2021-06-04T11:50:15Z

This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please complete review of the changes to move the PR forward. Thank you for your contributions.

jeromecoutant · 2021-06-04T15:25:43Z

@pennam did you have the chance to try #14733?
Thx

pennam · 2021-06-05T12:09:03Z

@jeromecoutant Yes, it works! I'm closing this pr because #14733 solves the issue.

Thanks.

Don't configure GPIO port pinout for _c pins

5423304

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label May 26, 2021

ciarmcom requested review from a team and jeromecoutant May 26, 2021 15:30

ciarmcom added needs: review devices: st labels May 26, 2021

pennam mentioned this pull request May 26, 2021

Breakout ETH arduino-libraries/Arduino_PortentaBreakout#14

Merged

jeromecoutant reviewed May 26, 2021

View reviewed changes

jeromecoutant suggested changes Jun 1, 2021

View reviewed changes

jeromecoutant mentioned this pull request Jun 3, 2021

STM32H7 ADC : dual pad feature update #14733

Merged

ciarmcom added the stale Stale Pull Request label Jun 4, 2021

adbridge added needs: work and removed needs: review labels Jun 4, 2021

ciarmcom removed the stale Stale Pull Request label Jun 4, 2021

pennam closed this Jun 5, 2021

mergify bot removed devices: st needs: work release-type: patch Indentifies a PR as containing just a patch labels Jun 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

STM32H7: Don't configure GPIO port pinout for _c pins #14707

STM32H7: Don't configure GPIO port pinout for _c pins #14707

Uh oh!

pennam commented May 26, 2021

Uh oh!

ciarmcom commented May 26, 2021

Uh oh!

jeromecoutant May 26, 2021

Uh oh!

pennam May 27, 2021

Uh oh!

jeromecoutant May 28, 2021

Uh oh!

jeromecoutant May 28, 2021

Uh oh!

pennam commented May 31, 2021

Uh oh!

jeromecoutant commented Jun 1, 2021

Uh oh!

jeromecoutant left a comment

Uh oh!

ciarmcom commented Jun 4, 2021

Uh oh!

jeromecoutant commented Jun 4, 2021

Uh oh!

pennam commented Jun 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

STM32H7: Don't configure GPIO port pinout for _c pins #14707

STM32H7: Don't configure GPIO port pinout for _c pins #14707

Uh oh!

Conversation

pennam commented May 26, 2021

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented May 26, 2021

Uh oh!

jeromecoutant May 26, 2021

Choose a reason for hiding this comment

Uh oh!

pennam May 27, 2021

Choose a reason for hiding this comment

Uh oh!

jeromecoutant May 28, 2021

Choose a reason for hiding this comment

Uh oh!

jeromecoutant May 28, 2021

Choose a reason for hiding this comment

Uh oh!

pennam commented May 31, 2021

Uh oh!

jeromecoutant commented Jun 1, 2021

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

ciarmcom commented Jun 4, 2021

Uh oh!

jeromecoutant commented Jun 4, 2021

Uh oh!

pennam commented Jun 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants