Implemement PxSEL setting into pinMode Function

[StefanSch]

Hi Team,

i am not sure if the implementation of the handling of the PxSEL bits is mode in the best way.
After some analysis i would like to make an suggestion for a change see below.
(Sore for the long thread !!!)

It basically implements the full functionality into the pinMode function.

It would also allow to have a define in the device specific header file (pins_energia.h) file which define how the PxSEL bits have to be set to enable a certain function e.g. UART RX

usage:

pinMode (UARTRXD, UARTRXD_SET_MODE)
pinMode (UARTTXD, UARTTXD_SET_MODE)
pinMode (TWISDA, TWISDA_SET_MODE)
pinMode (TWISCL, TWISCL_SET_MODE)

Required changes to files:

pins_energia.h
=================
static const uint8_t TWISDA  = 14;  /* P1.6 */
static const uint8_t TWISCL  = 15;  /* P1.7 */
static const uint8_t UARTRXD = 3;  /* Receive  Data (RXD) at P1.1 */
static const uint8_t UARTTXD = 4;  /* Transmit Data (TXD) at P1.2 */
#define TWISDA_SET_MODE  (PORT_SELECTION * 1 | INPUT_PULLUP)
#define TWISCL_SET_MODE  (PORT_SELECTION * 1 | INPUT_PULLUP)
#define UARTRXD_SET_MODE (PORT_SELECTION * 1 | INPUT)
#define UARTTXD_SET_MODE (PORT_SELECTION * 1 | OUTPUT)

Energia.h

#define INPUT 0x0
#define OUTPUT 0x1
#define INPUT_PULLUP 0x2
#define INPUT_PULLDOWN 0x4
#define PORT_SELECTION 0x10
#define PORT_SELECTION0 0x10
#define PORT_SELECTION1 0x20

wiring_digital.c

void pinMode(uint8_t pin, uint8_t mode)
{
    uint8_t bit = digitalPinToBitMask(pin);
    uint8_t port = digitalPinToPort(pin);

    volatile uint8_t *dir;
    volatile uint8_t *ren;
    volatile uint8_t *out;
    volatile uint8_t *sel;

    if (port == NOT_A_PORT) return;

    dir = portDirRegister(port);
    ren = portRenRegister(port);
    out = portOutputRegister(port);

    if (mode == INPUT) {
        *dir &= ~bit;
    } else if (mode == INPUT_PULLUP) {
        *dir &= ~bit;
                *out |= bit;
                *ren |= bit;
        } else if (mode == INPUT_PULLDOWN) {
        *dir &= ~bit;
                *out &= ~bit;
                *ren |= bit;
        } else {
        *dir |= bit;
    }

    #if (defined(P1SEL0_) || defined(P1SEL_) || defined(PASEL0_)) && (defined(UART_SET_PxSEL) || defined(UART_SET_PxSEL0))
    sel = portSel0Register(port);   /* get the port function select register address */
    if (mode & PORT_SELECTION0) {
        *sel |= bit;
    } else {
        *sel &= ~bit;
    }
    #endif
    #if (defined(P1SEL1_) || defined(P1SEL2_) || defined(PASEL1_)) && (defined(UART_SET_PxSEL1) || defined(UART_SET_PxSEL2))
    sel = portSel1Register(port);   /* get the port function select register address */
    if (mode & PORT_SELECTION1) {
        *sel |= bit;
    } else {
        *sel &= ~bit;
    }
    #endif


}

[robertinant]

@StefanSch, thanks for the suggestion and the detailed analysis. Will review it over the weekend.

[robertinant]

@StefanSch, where would the UART_SET_PxSEL2 and UART_SET_PxSEL1 come from and why the check for these? It seems that if a check needs to be done for these then a check might also be necessary for other types of peripherals. If that is the case then this might become a very big #if statement?

Also, the primary purpose of this function is to set the pin to INPUT or OUTPUT. Overloading it with so that it allows to configure the pin for other modes might be overkill.

Thoughts?

Robert

[StefanSch]

Hi Robert,

i made a test implementation and uploaded it in the Branch: Branch_F2274_Fr57xx_support

May you can have a look there, i think this will make many things much more clear.
(I have tested the implementation there for the UART on F2274 and FR5739 to see if this works.
Not a big test but at least the basic functionally is there.)

Back to your questions:
the defines are in the pins_energia.h files
The if statement is quite small.

Best regards,
Stefan

[RickKimball]

I'm wondering what the purpose of this would be? digital_write is supposed to deal with I/O on the pin. If it is touches the PSELX registers, it is only to turn off the alternate functionality and return the pin back to GPIO mode. I'm guessing that if you are manipulating the PSELX pins, you are doing it to gain access to the alternate functions of the pins. I think this would be better served in some class that is dealing with that alternate functionality. For example, using the UART in the FR5739, you have to manipulate the PSEL to access the UART capability. I think it best to bury that type of register manipulation code in the HardwareSerial not exposing the PSEL functionality as a special case of the digitalWrite.

digitalWrite is slow enough already I'd hate to see it get any slower.

What is the kind of performance hit do we take if we add these extra PSEL tests to digitalWrite?

-rick

[StefanSch]

Hi Nick,

the critical point if working with several different MSP430 device and families is that the usage of the PxSEL register differs between them slightly. This is not to much but at some devices there are two registers at some they are called PxSEL and PxSEL2 others have PxSEL0.PxSEL1. If we can not centralize this to a unique function and have the definitions packed into the device specific pins_energia.h file, it would be required to handle all this differences in the individual module files using the PxSEL register ( UART / I2C - Wire / PWM - Tone / ....)
I think esp. for the long term maintenance it would be better to have this at one point, even Rob already convinced me not to use the pinMode function but maybe a seperate one. So that the pinMode function can still stay as compact and short as possible.
~ Stefan

[RickKimball]

What code would use this pinSelMode() function? Is this for end user usage? Or for internals?

[StefanSch]

I see this more for internal functions. Not sure if this should/need to be also provided to the end user.

[RickKimball]

For internal code, I would like to see us start to use more C++ templates to deal with this type of thing. It would be great if we could move run time code and logic into compile time logic. This is a perfect case for it. We know all the variables at compile time and adding lookup tables just steals bytes from the available flash space.