Pin Tools: generated code is a mess, not usable in own code

[danielholala]

Hello,

I was under the impression that the Config Tools, especially the Pins Tool, would create code that is usable in my own code in a reasonable way. 

Turns out, I was wrong. The code created is just for initializing pins configuration. It can not be used to access or control the pins or at least reference the pins.

The config tool's generated code is a disaster.

For example if a project needs to control a LED, a GPIO is configured as output and assigned an identifier with a descriptive name. The routing information in the tool will look like this (example "led_blinky" from LPC5526 SDK, typos retained):

The generated code in pin_mux.c defines only this variable:

gpio_pin_config_t LED_BULE_config = {
        .pinDirection = kGPIO_DigitalOutput,
        .outputLogic = 0U
    };

That is not enough information to control the GPIO.

Consequently, the code is required to define information related to the GPIO in a second file board.h:

#ifndef BOARD_LED_BLUE_GPIO
#define BOARD_LED_BLUE_GPIO GPIO
#endif
#define BOARD_LED_BLUE_GPIO_PORT 1U
#ifndef BOARD_LED_BLUE_GPIO_PIN
#define BOARD_LED_BLUE_GPIO_PIN 4U
#endif

To control the GPIO, led_blinky.c contains:

#define BOARD_LED_PORT BOARD_LED_BLUE_GPIO_PORT
#define BOARD_LED_PIN  BOARD_LED_BLUE_GPIO_PIN

GPIO_PortToggle(GPIO, BOARD_LED_PORT, 1u << BOARD_LED_PIN);

That's an unmaintainable mess.

What I would expect - at the very least - is the Pin Tool to generate a data structure that represents the complete configuration information, e.g.:

PINMux_t pinmuxing[] = {
  [PIN_LED_BLUE] = {1, 4,  PIN_TYPE_GPIO, IOCON_MODE_INACT | IOCON_FUNC1, PIN_DIR_OUT},	/* identifier: LED_BLUE */
};

A GPIO would be addressed using an enum defined through the identifier.

The call to toggle a pin could then look like:

GPIOMuxed_PortToggle(PIN_LED_BLUE);

As simple as that and maintainable.

Or, to keep compatibility with existing SDK code, Pins Tool could generate functions to provide the information required to call SDK functions:

GPIO_PortToggle(GPIO, PINMUX_GetPort(PIN_LED_BLUE), PINMUX_GetMask(PIN_LED_BLUE));

What do you think?

[Petr_H]

Hi,

I agree that it's not very straightforward and the the code in the led_blinky example is a little bit overcomplicated. But, the board.h and main routine content is not generated by the config tools and you don't have to use it this way.

The single write to a pin can be just :

GPIO_PinWrite(BOARD_LED_BLUE_GPIO, BOARD_LED_BLUE_GPIO_PORT, BOARD_LED_BLUE_GPIO_PIN, 1);

Where LED_BLUE is the identifier you can put in the routing table. Expecting that you include pin_mux.h in your source module.

I agree that it would be in many cases easier having some structure holding the parameters, but as far as I know, the GPIO driver aimed to maximize the performance and effectivity of execution avoiding any additional operation.  

If you would like to simplify usage, you can define the following macro using macro concatenation to access the pin properties:

#define pinWrite(id, value) (GPIO_pinWrite(id##_GPIO,id##_PORT, id##_PIN, value))

Then in the code you can just use it like this:

pinWrite(BOARD_LED_BLUE, 1);

Additional note: The BOARD_ prefix can be customized if you'd be interested. It's in the functional group properties (Accessible using menu Pins > Functional groups or via the icon on the toolbar).

best regards

Petr Hradsky

Config Tools Team

[danielholala]

Dear  @Petr_H 

Thank you for your helpful reply.

The preprocessor macro that you suggested makes good use of the many defines the Pins tool creates, allows for code that is easier to maintain and to read. I wonder how many developer use it to cope with the output of the Pins tool?

That being said, this macro magic is just a workaround for what I think is a major shortcoming of the Pins tool and the SDK. Both should work together and provide a framework that makes programming easier and code more maintainable.

At the moment, they don't live up to this promise.

What you stated about the GPIO driver sounds like premature optimization. Not every application needs GPIO methods optimized for speed. Today, code readability and maintainability is often more important. I agree that in some applications there may be conflicting constraints regarding design time and runtime. But they don't need to be at odds as that's where the Pins tools & SDK should shine:  From the design side, by providing a consistent API and configuration to the developer that is easy to use and generated using the appropriate tools. From the runtime side a set of methods and defines to access methods directly.

For example, I'd like my application to output a list of all GPIOs (for debugging purposes) and if I add a GPIO, it should appear in this list without any manual code changes. As I understand, that's currently not possible. However, if the Pins tool would provide the configuration in pin_mux.h, code could simply iterate over it and list all GPIOs, their respective status, etc.

So please have the Pins tool expose the configuration in  pin_mux.h , e.g., as an array so user code can iterate over it. 

I hope this feature request make sense to you, too.

[Petr_H]

Regarding the optimization - I agree that not all application needs fast I/O, however offering a IO operation with the maximum effectiveness is the requirement that we need to fulfill. However, I also see benefits that would simpler API bring. I'll pass your feedback to the SDK team.

I'd like to clarify you request - what exact information you would like to have exposed in the proposed array ? Just mapping of GPIO/PORT/PIN information or something additional?

best regards

Petr Hradsky

Config Tools Team

[danielholala]

Hello @Petr_H ,

Thank you for getting back to me.

At the very least, I'd like to have basic pin mapping information (package pin number, port number, pin number on port, peripheral type, direction and identifier) in the proposed array. Of course, if space permits, I'd vote to have all the other properties, i.e., initial state, pulldown mode, slew rate, invert, open drain, for GPIOs included as well.

Further, the array entries should be directly accessible using (enum) defines.

Example:

enum {
  PINS_TXD3, PINS_RXD3, PINS_OFF, PINS_LIST_END
}

const Board_PinMuxList_t pinmuxing[] = {
  [PINS_TXD3] = {24, 0, 0, "TXD3",  PINS_IO_TYPE_SERIAL, PINS_IO_DIR_NOTREQUIRED },
  [PINS_RXD3] = {25, 0, 1, "RXD3",  PINS_IO_TYPE_SERIAL, PINS_IO_DIR_NOTREQUIRED},
  [PINS_OFF]  = {54, 1, 9, "OFF", PINS_IO_TYPE_GPIO  , PINS_IO_DIR_OUTPUT, PINS_IO_OPENDRAIN_ENABLED | PINS_IO_FLAGS_INIT_HIGH }
};

Does that make sense to you?

Best regards,
Dan

[Petr_H · Accepted solution]

Hi, 

Regarding basic mapping of port and pin, I think it's clear. Regarding all the individual properties, I wonder that it can take quite a lot of space in the memory. What can make sense is optionally make public the constant structures, that are already in the generated code. 

For example on the Kinetis K64, there are structures like this for each pin:

    const port_pin_config_t SW2 = {/* Internal pull-up resistor is enabled */
                                   kPORT_PullUp,
                                   /* Fast slew rate is configured */
                                   kPORT_FastSlewRate,
                                   /* Passive filter is disabled */
                                   kPORT_PassiveFilterDisable,
                                   /* Open drain is disabled */
                                   kPORT_OpenDrainDisable,
                                   /* Low drive strength is configured */
                                   kPORT_LowDriveStrength,
                                   /* Pin is configured as PTC6 */
                                   kPORT_MuxAsGpio,
                                   /* Pin Control Register fields [15:0] are not locked */
                                   kPORT_UnlockRegister};
    /* PORTC6 (pin 78) is configured as PTC6 */
    PORT_SetPinConfig(BOARD_SW2_PORT, BOARD_SW2_PIN, &SW2);

Would making such structures accessible suite your needs? (We would need to use unique identifier, putting them outside the function and add reference so some "list of pins") like this:

const Board_PinMuxList_t pinmuxing[] = {
  [PIN_SW2] = {BOARD_SW2_PORT, BOARD_SW2_PIN, &BOARD_SW2_PIN_CONFIG },
...

Additional note - it could take significant time to agree and implement such feature. If you'd need to have something in place, you could also use the export in CSV (File > Export > Pins tool / Export Pins  in CSV... It exports complete list of all pins and routing in comma separated text file, that you can simply process this list using for example python script and generate C source tailored for your needs.

Regards

Petr Hradsky

Config tools team

[danielholala]

Looks great. Yes, that's all it needs.   I think that solution has a very small memory footprint as it uses pointers to already allocated memory.

When you are discussing this feature, please also consider adding the package pin # and the identifier to the array as well.

I appreciate your support very much. It's great to witness that customer's thoughts and issues are noticed and responded to by the vendor. Thanks for listening. 

Best regards from Germany,

Dan

[Alice_Yang]

Hello danielholala,

About the Pin configuration, the generate code is in pin_mux.c and pin_mux.h files.

About toggle function, we can directly call gpio driver under SDK. 

BR

Alice