analogdevicesinc / msdk

Software Development Kit for Analog Device's MAX-series microcontrollers
Apache License 2.0
60 stars 76 forks source link

chore(PeriphDrivers): Define `MXC_GPIO_PAD` Enum Consistently Across All Parts #866

Closed ozersa closed 7 months ago

ozersa commented 7 months ago

Description

Most of MAX32 MCUs has pull-up/down and weak pull-up/down feature But some of MCUs only has pull-up/down, this differentiation create delta between gpio interface function and so that it make it hard to develop high level common interface functions.

The purpose of this commit is to align padding definition to provide common interface for gpio.h file

Without this addition it requires add extra macro/definition... to develop common fw which drive MAX32 MCUs, Like below

image

Checklist Before Requesting Review

ozersa commented 7 months ago

I agree this is better than the #ifdefs in the top-level code.

I think it might be better to throw E_NOT_SUPPORTED in GPIO_Config though, so users don't build on top of functionality that isn't there.

Would this be compatible with what you're working on?

// gpio.h
typedef enum {
    MXC_GPIO_PAD_NONE, ///< No pull-up or pull-down
    MXC_GPIO_PAD_PULL_UP, ///< Set pad to weak pull-up
    MXC_GPIO_PAD_PULL_DOWN, ///< Set pad to weak pull-down
    MXC_GPIO_PAD_WEAK_PULL_UP, ///< Set pad to weak pull-up
    MXC_GPIO_PAD_WEAK_PULL_DOWN, ///< Set pad to weak pull-down
} mxc_gpio_pad_t;
//gpio_me11.c
int MXC_GPIO_Config(const mxc_gpio_cfg_t *cfg)
{
    // ...

    // Configure the pad
    switch (cfg->pad) {
    case MXC_GPIO_PAD_NONE:
        gpio->pad_cfg1 &= ~cfg->mask;
        break;

    case MXC_GPIO_PAD_PULL_UP:
        gpio->pad_cfg1 |= cfg->mask;
        gpio->ps |= cfg->mask;
        break;

    case MXC_GPIO_PAD_PULL_DOWN:
        gpio->pad_cfg1 |= cfg->mask;
        gpio->ps &= ~cfg->mask;
        break;

    case MXC_GPIO_PAD_WEAK_PULL_UP:
    case MXC_GPIO_PAD_WEAK_PULL_DOWN:
        return E_NOT_SUPPORTED;

    default:
        return E_BAD_PARAM;
    }

Yes it will works for me but to be honest I prefer fist one, because I believe returning E_NOT_SUPPORT solution will be hide things from user due to interface file says it support weak option but without checking source code (.c) or build&execution firmware the user will not able to evaluate code behaviour. If interface file (gpio.h) be common between devices, returning E_NOT_SUPPORT would be better but interface file device specific. By proposed solution in this PR user will see that there is not differentiation between weak/strong pullup on the device so I believe it will be more speaking code.

Jake-Carter commented 7 months ago

I agree this is better than the #ifdefs in the top-level code. I think it might be better to throw E_NOT_SUPPORTED in GPIO_Config though, so users don't build on top of functionality that isn't there. Would this be compatible with what you're working on?

// gpio.h
typedef enum {
    MXC_GPIO_PAD_NONE, ///< No pull-up or pull-down
    MXC_GPIO_PAD_PULL_UP, ///< Set pad to weak pull-up
    MXC_GPIO_PAD_PULL_DOWN, ///< Set pad to weak pull-down
    MXC_GPIO_PAD_WEAK_PULL_UP, ///< Set pad to weak pull-up
    MXC_GPIO_PAD_WEAK_PULL_DOWN, ///< Set pad to weak pull-down
} mxc_gpio_pad_t;
//gpio_me11.c
int MXC_GPIO_Config(const mxc_gpio_cfg_t *cfg)
{
    // ...

    // Configure the pad
    switch (cfg->pad) {
    case MXC_GPIO_PAD_NONE:
        gpio->pad_cfg1 &= ~cfg->mask;
        break;

    case MXC_GPIO_PAD_PULL_UP:
        gpio->pad_cfg1 |= cfg->mask;
        gpio->ps |= cfg->mask;
        break;

    case MXC_GPIO_PAD_PULL_DOWN:
        gpio->pad_cfg1 |= cfg->mask;
        gpio->ps &= ~cfg->mask;
        break;

    case MXC_GPIO_PAD_WEAK_PULL_UP:
    case MXC_GPIO_PAD_WEAK_PULL_DOWN:
        return E_NOT_SUPPORTED;

    default:
        return E_BAD_PARAM;
    }

Yes it will works for me but to be honest I prefer fist one, because I believe returning E_NOT_SUPPORT solution will be hide things from user due to interface file says it support weak option but without checking source code (.c) or build&execution firmware the user will not able to evaluate code behaviour. If interface file (gpio.h) be common between devices, returning E_NOT_SUPPORT would be better but interface file device specific. By proposed solution in this PR user will see that there is not differentiation between weak/strong pullup on the device so I believe it will be more speaking code.

Sure, these are valid points. My solution pushes the error to run-time. I agree your proposed solution is more clear when writing the code, let's merge your method