HAL_GPIO_PIN

[MonteShaffer]

Hi Alex,

I wanted to provide an update of HAL_GPIO_PIN based on the initial code base you provided. Specifically I wanted to "bind" the functions to a global structure...

#include <stdint.h>
#include <stdbool.h>

#ifndef _HAL_GPIO_H_
#define _HAL_GPIO_H_

/*- Definitions -------------------------------------------------------------*/
#define HAL_GPIO_PORTA       0
#define HAL_GPIO_PORTB       1  
#define HAL_GPIO_PORTC       2

#define HAL_GPIO_PMUX_A      0
#define HAL_GPIO_PMUX_B      1 /* see Table 7-1 of datasheet:: all ADCs are on PMUX_B for SAMD21 */
#define HAL_GPIO_PMUX_C      2
#define HAL_GPIO_PMUX_D      3
#define HAL_GPIO_PMUX_E      4
#define HAL_GPIO_PMUX_F      5
#define HAL_GPIO_PMUX_G      6
#define HAL_GPIO_PMUX_H      7
#define HAL_GPIO_PMUX_I      8

typedef struct ADC_pin {
	char * pin_name;
	char pin_port;
	uint8_t pin_num;
	uint8_t pin_ref;
	uint8_t pmux;
	void (*set)(void);
  void (*clr)(void);
  void (*toggle)(void);
  void (*write)(int);
  void (*input)(void);
  void (*output)(void);
  void (*pullen)(int);
  int (*read)(void);
  int (*state)(void);
  void (*pmuxen)(int);
  void (*pmuxdis)(void);
	uint8_t has_init;
} ADC_pin;

/* _ref_ is in Table 7-1, represents AIN[bracket PIN number] */
#define SURROUND_H(q,c) q##c##q
#define QUOTE '
#define SURROUND(q,c) SURROUND_H(q,c)
#define CHARIFY(c) SURROUND(QUOTE, c)
//#define HAL_GPIO_ADC(name, port, pin, ...)	
#define HAL_GPIO_PIN(name, port, pin, pmux_val, _ref_)						\
  static inline void HAL_GPIO_##name##_set(void)				\
  {										\
    PORT->Group[HAL_GPIO_PORT##port].OUTSET.reg = (1 << pin);			\
    (void)HAL_GPIO_##name##_set;						\
  }										\
										\
  static inline void HAL_GPIO_##name##_clr(void)				\
  {										\
    PORT->Group[HAL_GPIO_PORT##port].OUTCLR.reg = (1 << pin);			\
    (void)HAL_GPIO_##name##_clr;						\
  }										\
										\
  static inline void HAL_GPIO_##name##_toggle(void)				\
  {										\
    PORT->Group[HAL_GPIO_PORT##port].OUTTGL.reg = (1 << pin);			\
    (void)HAL_GPIO_##name##_toggle;						\
  }										\
										\
  static inline void HAL_GPIO_##name##_write(int value)				\
  {										\
    if (value)									\
      PORT->Group[HAL_GPIO_PORT##port].OUTSET.reg = (1 << pin);			\
    else									\
      PORT->Group[HAL_GPIO_PORT##port].OUTCLR.reg = (1 << pin);			\
    (void)HAL_GPIO_##name##_write;						\
  }										\
										\
  static inline void HAL_GPIO_##name##_input(void)					\
  {										\
    PORT->Group[HAL_GPIO_PORT##port].DIRCLR.reg = (1 << pin);			\
    PORT->Group[HAL_GPIO_PORT##port].PINCFG[pin].reg |= PORT_PINCFG_INEN;	\
    (void)HAL_GPIO_##name##_input;							\
  }										\
										\
  static inline void HAL_GPIO_##name##_output(void)				\
  {										\
    PORT->Group[HAL_GPIO_PORT##port].DIRSET.reg = (1 << pin);			\
    PORT->Group[HAL_GPIO_PORT##port].PINCFG[pin].reg |= PORT_PINCFG_INEN;	\
    (void)HAL_GPIO_##name##_output;						\
  }										\
										\
  static inline void HAL_GPIO_##name##_pullen(int state)			\
  {										\
    if (state)									\
      PORT->Group[HAL_GPIO_PORT##port].PINCFG[pin].reg |= PORT_PINCFG_PULLEN;	\
    else									\
      PORT->Group[HAL_GPIO_PORT##port].PINCFG[pin].reg &= ~PORT_PINCFG_PULLEN;	\
    (void)HAL_GPIO_##name##_pullen;						\
  }										\
										\
  static inline int HAL_GPIO_##name##_read(void)				\
  {										\
    return (PORT->Group[HAL_GPIO_PORT##port].IN.reg & (1 << pin)) != 0;		\
    (void)HAL_GPIO_##name##_read;						\
  }										\
										\
  static inline int HAL_GPIO_##name##_state(void)				\
  {										\
    return (PORT->Group[HAL_GPIO_PORT##port].DIR.reg & (1 << pin)) != 0;	\
    (void)HAL_GPIO_##name##_state;						\
  }										\
										\
  static inline void HAL_GPIO_##name##_pmuxen(int mux)				\
  {										\
    PORT->Group[HAL_GPIO_PORT##port].PINCFG[pin].reg |= PORT_PINCFG_PMUXEN;	\
    if (pin & 1)								\
      PORT->Group[HAL_GPIO_PORT##port].PMUX[pin>>1].bit.PMUXO = mux;		\
    else									\
      PORT->Group[HAL_GPIO_PORT##port].PMUX[pin>>1].bit.PMUXE = mux;		\
    (void)HAL_GPIO_##name##_pmuxen;						\
  }										\
										\
  static inline void HAL_GPIO_##name##_pmuxdis(void)				\
  {										\
    PORT->Group[HAL_GPIO_PORT##port].PINCFG[pin].reg &= ~PORT_PINCFG_PMUXEN;	\
    (void)HAL_GPIO_##name##_pmuxdis;						\
  }										\
		\
	 ADC_pin g_##name = {	\
	.pin_name = #name, \
	.pin_port = CHARIFY(port), \
	.pin_num = pin, \
	.pin_ref = _ref_,					\
	.pmux = pmux_val,	\
	.set = &HAL_GPIO_##name##_set,						\
	.clr = &HAL_GPIO_##name##_clr,						\
	.toggle = &HAL_GPIO_##name##_toggle,						\
	.write = &HAL_GPIO_##name##_write,						\
	.input = &HAL_GPIO_##name##_input,						\
	.output = &HAL_GPIO_##name##_output,						\
	.pullen = &HAL_GPIO_##name##_pullen,						\
	.read = &HAL_GPIO_##name##_read,						\
	.state = &HAL_GPIO_##name##_state,						\
	.pmuxen = &HAL_GPIO_##name##_pmuxen,						\
	.pmuxdis = &HAL_GPIO_##name##_pmuxdis,						\
	.has_init = 0 }

#endif

Thereafter, adc.h has some updated functions ...

#include <stdint.h>
#include "hal_gpio.h"

#ifndef _ADC_H_
#define _ADC_H_




/*- Prototypes --------------------------------------------------------------*/
void adc_deinit(ADC_pin * myPIN);
void adc_init(ADC_pin * myPIN);
void adc_update(ADC_pin * myPIN);
int adc_read(void);
int getVoltageFromADC(ADC_pin * myPIN);


#endif // _ADC_H_

which are detailed in adc.c

#include <stdlib.h>
#include <stdint.h>
#include <stdbool.h>
#include "samd21.h"
#include "hal_gpio.h"
#include "nvmdata.h"
#include "adc.h"

void adc_deinit(ADC_pin * myPIN) {
	myPIN->has_init = 0;
}

void adc_init(ADC_pin * myPIN) {
	if(!myPIN->has_init) {
		myPIN->input();
		myPIN->pmuxen(myPIN->pmux);
		myPIN->has_init = 1;
	}
	
}

void adc_update(ADC_pin * myPIN) {
	//adc_init();
	PM->APBCMASK.reg |= PM_APBCMASK_ADC;

  GCLK->CLKCTRL.reg = GCLK_CLKCTRL_ID(ADC_GCLK_ID) |
      GCLK_CLKCTRL_CLKEN | GCLK_CLKCTRL_GEN(0);

  ADC->CTRLA.reg = ADC_CTRLA_SWRST;
  while (ADC->CTRLA.reg & ADC_CTRLA_SWRST);

  ADC->REFCTRL.reg = ADC_REFCTRL_REFSEL_INTVCC1 | ADC_REFCTRL_REFCOMP;
  ADC->CTRLB.reg = ADC_CTRLB_RESSEL_16BIT | ADC_CTRLB_PRESCALER_DIV32;
  ADC->AVGCTRL.reg = ADC_AVGCTRL_SAMPLENUM_128;
  ADC->INPUTCTRL.reg = myPIN->pin_ref | ADC_INPUTCTRL_MUXNEG_GND | ADC_INPUTCTRL_GAIN_DIV2;
	//ADC->INPUTCTRL.reg = HAL_GPIO_PINA3_getMyVal() | ADC_INPUTCTRL_MUXNEG_GND | ADC_INPUTCTRL_GAIN_DIV2;
	ADC->CALIB.reg = ADC_CALIB_BIAS_CAL(NVM_READ_CAL(ADC_BIASCAL)) |
	ADC_CALIB_LINEARITY_CAL(NVM_READ_CAL(ADC_LINEARITY));

	ADC->CTRLA.reg = ADC_CTRLA_ENABLE;
}

int adc_read(void)
{
  ADC->SWTRIG.reg = ADC_SWTRIG_START;
  while (0 == (ADC->INTFLAG.reg & ADC_INTFLAG_RESRDY));

  int result = ADC->RESULT.reg;
	return result;
}

Altogether, I now have my function I wanted ...

int getVoltageFromADC(ADC_pin * myPIN);

int getVoltageFromADC(ADC_pin * myPIN) {
	adc_init(myPIN);
	adc_update(myPIN);
	return adc_read();
}

Which is initiated as follows:

HAL_GPIO_PIN(PINA3, A, 3, HAL_GPIO_PMUX_B, ADC_INPUTCTRL_MUXPOS_PIN1);

// above passes more parameters and makes a global structure with pointers to the functions defined therein.  The structure name is g_PINA3.  The functions are bound to this struct

int cvolt = getVoltageFromADC(g_PINA3);

I like the HAL_GPIO_PIN, and now realize you use it for all pins, LED/DAC, not juse ADC... we may want to update our structure to "replace ADC_PIN with GPIO_PIN" in the struct

I changed _in() and _out() to _input() and _output()

... we tried to make the macro variadic, but didn't seem worth the trouble, so we passes MUX, PIN_REF as extra parameters...

Comments are welcome...

[ataradov]

This is a horrible way to write the code.

You just moved platform specific stuff into your application code. Applications should operate on virtual channels numbered 0 to N. The low level code should know about mapping of the channel to the pin. This way you will have to port only one file when changing platforms.

All initialization code must run on startup, not first use. Otherwise you are creating potential problems with timings, since the first read will take longer and if you move the code with that first read around, you will have unpredictable timings.

Your application code should look like this:

#define APP_TEMPERATURE 0
#define APP_VOLTAGE 1
#define APP_CURRRENT 2

adc_init();
int voltage = adc_read_channel(APP_VOLTAGE);

Or if you have very few channels:
int voltage = adc_read_voltage();
int temperature = adc_read_temperature();

Looks like a lot duplicate code, right? Well try to rewrite this code this way and see how shorter and cleaner it gets.

Typically people write the code this way when they feel like writing, but don't have an actual problem to solve, so they write bloated "libraries" like this.

[MonteShaffer]

Hi Surich,

I appreciate your comments.

HAL_GPIO_PIN is intended to be a "hardware abstraction layer". Yes? If so, why would it not capture everything related to a GPIO pin. ADC pins, also have a AIN pin_ref. So should HAL_GPIO_PIN take care of this?

My approach

HAL_GPIO_PIN(PINA3, A, 3, HAL_GPIO_PMUX_B, ADC_INPUTCTRL_MUXPOS_PIN1);

defines the information in the macro, where, IMO, it should be defined. It creates the appropriate functions and allows me to assign the appropriate maps in one location.

HAL_GPIO_PIN(PINA3, A, 3);

Your approach will require me to lookup HAL_GPIO_PMUX_B and ADC_INPUTCTRL_MUXPOS_PIN1 at some later point.

Within main I can call all of the pins I will need, init them, and move on.

In my current workup, I will have two pins for reading rotator data (linear potentiometer).

HAL_GPIO_PIN(VOLTAGE_POTENTIOMETER1, A, 3, HAL_GPIO_PMUX_B, ADC_INPUTCTRL_MUXPOS_PIN1);
HAL_GPIO_PIN(VOLTAGE_POTENTIOMETER2, A, 2, HAL_GPIO_PMUX_B, ADC_INPUTCTRL_MUXPOS_PIN1);

Then, I have an LED, and DEBUG using UART_TX and UART_RX ... that gives me 5 total HAL objects. I plan on adding an external RGB LED on a pin. Now 6 HAL objects. When I compile, I am about 2K.

That doesn't even include any ISR pins yet.

I am letting the define macro write all of the code for me and store in a data structure g_##name

Anyway, I would challenge you to compare you code to mine. A bloated library is something that is abstract for the sake of being abstract. Your sample code was hardcoded (remember ADC) which is far from being a useful adc.h and adc.c so I reached out to you to share my ideas. My approach is not theoretical, it is very practical. We are developing a system with multiple peripherals to help people with Parkinson's Disease. I can possibly have more that one ADC and the SAMD21 permits this. It is very plausible that I need six VOLTAGE ADC objects. If I use your approach, I have to keep track of all of the mappings in all of the locations. A little more design can go a long way...

Thanks for your comment about the timings, we will make certain everything _init() at the beginning of main so the GCLK timings are fine.

"Hacking" a solution is not designing good code to solve a problem. I am trying to do the latter that will achieve the goals of the former.

Creating embedded software that doesn't enable full functionality of the device, IMO, would be a better definition of "horrible."

Look at adc_init() for your code... How would I tell A03 to link to ADC_INPUTCTRL_MUXPOS_PIN1 ? Your solution was to create another universe of parallel variables

static int g_channels[] = {ADC_INPUTCTRL_MUXPOS_PIN1, ADC_INPUTCTRL_MUXPOS_PIN4, ADC_INPUTCTRL_MUXPOS_PIN5, ADC_INPUTCTRL_MUXPOS_PIN6 };

In this new example, you don't call _in(); with your _pmuxen()l

My idea is to wrap all of the necessary information for a GPIO in a single structure. I believe HAL stands for "hardware abstraction layer" and not for "hardly able lazy" ...

Из двух зол выбира́ют ме́ньшее.

Anyway, I appreciate you sharing your code base, I like how you organize objects, and we will try to use it as a foundation to do more. Thank you!

[ataradov]

What happens if you need different gain on different channels, or one of the channels to be differential? My arrays do this easily.

Dereferencing all those structures also slows down run-time performance and consumes extra RAM, but in reality none of them will ever change once application is compiled.

And sometimes I enable and disable PMUX in run-time depending on the code flow.

Do whatever works for you, of course. My experience tells me that this is not a way to go.

[ataradov]

Here is a practical example of such structures that are taken from a big real projects (some things are renamed for privacy):

typedef struct
{
  int          index;
  uint32_t     inputs;
  uint32_t     ref;
  uint32_t     mode;

  uint32_t     calib_inputs;
  int32_t      calib_avg;
  int32_t      calib_offset;
} adc_channel_t;

static const adc_channel_t adc_channels[] =
{
  {
    ADC_INDEX_XXX_1,
    ADC_INPUTCTRL_MUXPOS_PIN8 | ADC_INPUTCTRL_MUXNEG_PIN6 | ADC_INPUTCTRL_GAIN_DIV2,
    ADC_REFCTRL_REFSEL_INTVCC0,
    0,
    ADC_NO_CALIBRATION,
    0, 0
  },
  {
    ADC_INDEX_XXX_2,
    ADC_INPUTCTRL_MUXPOS_PIN9 | ADC_INPUTCTRL_MUXNEG_PIN7 | ADC_INPUTCTRL_GAIN_DIV2,
    ADC_REFCTRL_REFSEL_INTVCC0,
    0,
    ADC_NO_CALIBRATION,
    0, 0
  },
  {
    ADC_INDEX_YYY,
    ADC_INPUTCTRL_MUXPOS_PIN12 | ADC_INPUTCTRL_MUXNEG_PIN4 | ADC_INPUTCTRL_GAIN_DIV2,
    ADC_REFCTRL_REFSEL_INTVCC0,
    0,
    ADC_NO_CALIBRATION,
    0, 0
  },
  {
    ADC_INDEX_ZZZ,
    ADC_INPUTCTRL_MUXPOS_PIN13 | ADC_INPUTCTRL_MUXNEG_PIN5 | ADC_INPUTCTRL_GAIN_16X,
    ADC_REFCTRL_REFSEL_INT1V,
    ADC_CTRLB_DIFFMODE,
    ADC_INPUTCTRL_MUXPOS_PIN5 | ADC_INPUTCTRL_MUXNEG_PIN5 | ADC_INPUTCTRL_GAIN_16X,
    0, 0
  },
  { ADC_INDEX_END, 0, 0, 0, 0, 0, 0 },
};

As you can see, I scan 4 virtual channels with various gains, some in differential mode, some use additional calibration and different references.

[MonteShaffer]

Thanks Alex for a glimpse of your sample code. I believe a structure approach is appropriate, and adding "calibration" and "gains" like you suggest is very doable.

Our current design goal is a 10-ADC system with 10-RGB LED programmable lights (WS2812b or SK6812) all running on the SAMD21 with a few ISRs to update the state of each ADC peripheral. A single RGB will be linked a specific ADC peripheral.

If you have any code for the SAMD21 for RGB LEDS in an array or even a hardware ISR component (e.g., linking a hardware response to EIC interrupt) please share, as it would benefit my team's progress.

Спасибо

[ataradov]

I have never worked with WS2812 or any other programmable LEDs like this, but I know that people use I2S peripheral to drive them to minimize CPU intervention. You have to translate the bits you want to send to a series of 0s and 1s sent over I2S at a high frequency to form the required waveform. I2S can send up to 32 "fast" bits, so it saves quite a bit of work for the MCU.

Overall if this is all that is required, then I would just try the easiest approach to see if you run into any timing problems. I would not expect any.