Author Topic: BSRR in STM32F4xx.h  (Read 7865 times)

0 Members and 1 Guest are viewing this topic.

Offline lollandsterTopic starter

  • Contributor
  • Posts: 40
  • Country: no
BSRR in STM32F4xx.h
« on: April 08, 2019, 11:41:07 am »
Hi,
I'm having some trouble understanding the BSRR register in the STM32F4xx.h file.
The BSRR register is defined as two 16 bit registers (H and L) in the struct, but the BSRR bit definitions are 32 bit. What am I missing?
Doing GPIOA->BSRRL = GPIO_BSRR_BR_4 will put a 32bit value into a 16 bit space. That may work, but doesn't feel like the right thing to do. For now I'll just do GPIOA->BSRRH = ((uint16_t)0x0010), but I would like to know why the STM32F4xx.h is like it is.
In the STM32F30x.h the BSRR is 32 bit and makes sense.

Code: [Select]
typedef struct
{
  __IO uint32_t MODER;    /*!< GPIO port mode register,               Address offset: 0x00      */
  __IO uint32_t OTYPER;   /*!< GPIO port output type register,        Address offset: 0x04      */
  __IO uint32_t OSPEEDR;  /*!< GPIO port output speed register,       Address offset: 0x08      */
  __IO uint32_t PUPDR;    /*!< GPIO port pull-up/pull-down register,  Address offset: 0x0C      */
  __IO uint32_t IDR;      /*!< GPIO port input data register,         Address offset: 0x10      */
  __IO uint32_t ODR;      /*!< GPIO port output data register,        Address offset: 0x14      */
  __IO uint16_t BSRRL;    /*!< GPIO port bit set/reset low register,  Address offset: 0x18      */
  __IO uint16_t BSRRH;    /*!< GPIO port bit set/reset high register, Address offset: 0x1A      */
  __IO uint32_t LCKR;     /*!< GPIO port configuration lock register, Address offset: 0x1C      */
  __IO uint32_t AFR[2];   /*!< GPIO alternate function registers,     Address offset: 0x20-0x24 */
} GPIO_TypeDef;

Code: [Select]
/******************  Bits definition for GPIO_BSRR register  ******************/
#define GPIO_BSRR_BS_0                       ((uint32_t)0x00000001)
#define GPIO_BSRR_BS_1                       ((uint32_t)0x00000002)
#define GPIO_BSRR_BS_2                       ((uint32_t)0x00000004)
#define GPIO_BSRR_BS_3                       ((uint32_t)0x00000008)
#define GPIO_BSRR_BS_4                       ((uint32_t)0x00000010)
#define GPIO_BSRR_BS_5                       ((uint32_t)0x00000020)
#define GPIO_BSRR_BS_6                       ((uint32_t)0x00000040)
#define GPIO_BSRR_BS_7                       ((uint32_t)0x00000080)
#define GPIO_BSRR_BS_8                       ((uint32_t)0x00000100)
#define GPIO_BSRR_BS_9                       ((uint32_t)0x00000200)
#define GPIO_BSRR_BS_10                      ((uint32_t)0x00000400)
#define GPIO_BSRR_BS_11                      ((uint32_t)0x00000800)
#define GPIO_BSRR_BS_12                      ((uint32_t)0x00001000)
#define GPIO_BSRR_BS_13                      ((uint32_t)0x00002000)
#define GPIO_BSRR_BS_14                      ((uint32_t)0x00004000)
#define GPIO_BSRR_BS_15                      ((uint32_t)0x00008000)
#define GPIO_BSRR_BR_0                       ((uint32_t)0x00010000)
#define GPIO_BSRR_BR_1                       ((uint32_t)0x00020000)
#define GPIO_BSRR_BR_2                       ((uint32_t)0x00040000)
#define GPIO_BSRR_BR_3                       ((uint32_t)0x00080000)
#define GPIO_BSRR_BR_4                       ((uint32_t)0x00100000)
#define GPIO_BSRR_BR_5                       ((uint32_t)0x00200000)
#define GPIO_BSRR_BR_6                       ((uint32_t)0x00400000)
#define GPIO_BSRR_BR_7                       ((uint32_t)0x00800000)
#define GPIO_BSRR_BR_8                       ((uint32_t)0x01000000)
#define GPIO_BSRR_BR_9                       ((uint32_t)0x02000000)
#define GPIO_BSRR_BR_10                      ((uint32_t)0x04000000)
#define GPIO_BSRR_BR_11                      ((uint32_t)0x08000000)
#define GPIO_BSRR_BR_12                      ((uint32_t)0x10000000)
#define GPIO_BSRR_BR_13                      ((uint32_t)0x20000000)
#define GPIO_BSRR_BR_14                      ((uint32_t)0x40000000)
#define GPIO_BSRR_BR_15                      ((uint32_t)0x80000000)
 

Online newbrain

  • Super Contributor
  • ***
  • Posts: 1713
  • Country: se
Re: BSRR in STM32F4xx.h
« Reply #1 on: April 08, 2019, 12:36:03 pm »
That may work
It definitely won't, I'll refrain from quoting the standard as I usually do... :blah:

BUT:
Which version of the file are you using?
I'm asking as in the F4 Cube version I have (1.21) BSRR is a 32 bit register, the files where it's defined (which are processor specific and included by STM32F4xx.h) are dated 2017.

EtA: found it in the release notes:
Quote
V2.2.0 / 15-December-2014
Main Changes
stm32f4xx.h
[.....8<....]
GPIO_TypeDef: change the BSRR register definition, the two 16-bits definition BSRRH and BSRRL are merged in a single 32-bits definition BSRR
So, you might need to update your Cube...
« Last Edit: April 08, 2019, 12:39:50 pm by newbrain »
Nandemo wa shiranai wa yo, shitteru koto dake.
 

Offline iMo

  • Super Contributor
  • ***
  • Posts: 4670
  • Country: nr
  • It's important to try new things..
Re: BSRR in STM32F4xx.h
« Reply #2 on: April 08, 2019, 01:59:10 pm »
If you look carefully at below defs the address offset is 2 in this case (otherwise it will be 4 for 32bit):
Code: [Select]
  __IO uint16_t BSRRL;    /*!< GPIO port bit set/reset low register,  Address offset: 0x18      */
  __IO uint16_t BSRRH;    /*!< GPIO port bit set/reset high register, Address offset: 0x1A      */
 

Offline lollandsterTopic starter

  • Contributor
  • Posts: 40
  • Country: no
Re: BSRR in STM32F4xx.h
« Reply #3 on: April 08, 2019, 02:20:59 pm »
BUT:
Which version of the file are you using?
I'm asking as in the F4 Cube version I have (1.21) BSRR is a 32 bit register, the files where it's defined (which are processor specific and included by STM32F4xx.h) are dated 2017.

EtA: found it in the release notes:
Quote
V2.2.0 / 15-December-2014
Main Changes
stm32f4xx.h
[.....8<....]
GPIO_TypeDef: change the BSRR register definition, the two 16-bits definition BSRRH and BSRRL are merged in a single 32-bits definition BSRR
So, you might need to update your Cube...

My STM32F4xx.h file is marked V1.8.0 / 09-November-2016.
The CubeMX software has stopped working for some reason (Http access forbidden), probably some work related firewall issues. I downloaded the standard peripheral library using System Workbench.
Is there a better way to get the libraries?
 

Online newbrain

  • Super Contributor
  • ***
  • Posts: 1713
  • Country: se
Re: BSRR in STM32F4xx.h
« Reply #4 on: April 08, 2019, 03:08:12 pm »
You can directly download the last Cube  (not CubeMX), including HAL, CMSIS, examples etc. from st.com website.
AFAIU, the standard peripheral library is no longer supported, "replaced" by the LL.

E.g. this is the link for the F4 Cube.

If you look carefully at below defs the address offset is 2 in this case (otherwise it will be 4 for 32bit):
Code: [Select]
  __IO uint16_t BSRRL;    /*!< GPIO port bit set/reset low register,  Address offset: 0x18      */
  __IO uint16_t BSRRH;    /*!< GPIO port bit set/reset high register, Address offset: 0x1A      */
Yes, the register has not changed with the SW update  >:D. I don't know if correct #defines for a split BSRR were provided in some version :-//, but the ones shown by lollandster will not work.
Nandemo wa shiranai wa yo, shitteru koto dake.
 

Offline Yansi

  • Super Contributor
  • ***
  • Posts: 3893
  • Country: 00
  • STM32, STM8, AVR, 8051
Re: BSRR in STM32F4xx.h
« Reply #5 on: April 08, 2019, 03:33:41 pm »
This is a well known shithole.

Just live with it. Either write 32bit to the address of BSRRL, or write 16bit BRR content to BSRRH register.

Code: [Select]
/* Write to BSRR */
*(uint32_t*)&GPIOx->BSRRL =VALUEfor32bitBSRRreg;

//Or just modify the header file and forget it.
 

Offline iMo

  • Super Contributor
  • ***
  • Posts: 4670
  • Country: nr
  • It's important to try new things..
Re: BSRR in STM32F4xx.h
« Reply #6 on: April 08, 2019, 06:05:22 pm »
Doing GPIOA->BSRRL = GPIO_BSRR_BR_4 will put a 32bit value into a 16 bit space. That may work, but doesn't feel like the right thing to do.
Well, you normally do not write GPIO_BSRR_BR_4 into BSRRL.

The GPIO_BSRR_BR_4 is only the "bit name" of the BSRR register. Therefore 32 bit names.
Simply forget the GPIO_BSRR_BR_4 and a like bit names.
And do not modify header files unless you understand well what you are doing :).

You write an uint16 "mask" with bits you want to set or reset atomically to BSRRL (set) or BSRRH (reset).

GPIOA->BSRRL = 0b0000000001101000;
GPIOA->BSRRH = 0b0000000001101000;   // toggles A6,A5,A3
« Last Edit: April 08, 2019, 06:36:28 pm by imo »
 

Offline ajb

  • Super Contributor
  • ***
  • Posts: 2582
  • Country: us
Re: BSRR in STM32F4xx.h
« Reply #7 on: April 08, 2019, 06:12:20 pm »
I'm all for using named bitmask macros for situations where each bit has a different function, but for stuff like this where the bits reflect an array of the same function on different pins, it's easy enough just to do a bitshift based on

Code: [Select]
PORTx->BSRRL = (1<<pinNum)
Personally I just hide all that shit behind macros I can write once for each platform and then not have to think about ever again:
Code: [Select]
#define IO_OUTSET(...) /* set (port, pin) output */ IO_OUTSET_SUB(__VA_ARGS__)
#define IO_OUTCLR(...) /* clear (port, pin) output */ IO_OUTCLR_SUB(__VA_ARGS__)

#define IO_OUTSET_SUB(gpio, pin)                 GPIO##gpio->BSRRL = (1<<pin)
#define IO_OUTCLR_SUB(gpio, pin)                 GPIO##gpio->BSRRH = (1<<pin)

The double defines allow things like this to work:

Code: [Select]
#define LED_PIN    B,4
IO_OUTSET(LED_PIN)
 

Offline iMo

  • Super Contributor
  • ***
  • Posts: 4670
  • Country: nr
  • It's important to try new things..
Re: BSRR in STM32F4xx.h
« Reply #8 on: April 08, 2019, 06:27:41 pm »
The beauty of the BSRR is you can set/reset atomically (no read/modify/write) a "set of bits".

Something like:

#define LEDS_PINS    B,0,1,4,12,15
IO_OUTSET(LEDS_PINS)
IO_OUTCLR(LEDS_PINS)

should end as:

GPIOB->BSRRL = 0b1001000000010011;
GPIOB->BSRRH = 0b1001000000010011; 



« Last Edit: April 08, 2019, 06:38:51 pm by imo »
 

Offline Yansi

  • Super Contributor
  • ***
  • Posts: 3893
  • Country: 00
  • STM32, STM8, AVR, 8051
Re: BSRR in STM32F4xx.h
« Reply #9 on: April 08, 2019, 06:32:38 pm »
Please note that using POSITIONs insted of MASKs makes rather mess of a code, use the pre-defined GPIO_Pin_0..15 macros instead.
Using binary constants (and magic numbers anyway) shall be even punished or made illegal  :D

So instead of
GPIOA->BSRRL = 0x01101000;
GPIOA->BSRRH = 0x01101000;   // toggles A6,A5,A3

use please
GPIOA->BSRRL = GPIO_Pin_6 | GPIO_Pin_5 | GPIO_Pin_3;   /* sets A6, A5 and A3 */
GPIOA->BSRRH = GPIO_Pin_6 | GPIO_Pin_5 | GPIO_Pin_3;   /* clears A6, A5 and A3 */



 

Offline iMo

  • Super Contributor
  • ***
  • Posts: 4670
  • Country: nr
  • It's important to try new things..
Re: BSRR in STM32F4xx.h
« Reply #10 on: April 08, 2019, 06:46:01 pm »
@Yansi: your example above is a good example why it should be punished :) :)
 

Offline lucazader

  • Regular Contributor
  • *
  • Posts: 221
  • Country: au
Re: BSRR in STM32F4xx.h
« Reply #11 on: April 08, 2019, 07:31:57 pm »
Please note that using POSITIONs insted of MASKs makes rather mess of a code, use the pre-defined GPIO_Pin_0..15 macros instead.
Using binary constants (and magic numbers anyway) shall be even punished or made illegal  :D

So instead of
GPIOA->BSRRL = 0x01101000;
GPIOA->BSRRH = 0x01101000;   // toggles A6,A5,A3

use please
GPIOA->BSRRL = GPIO_Pin_6 | GPIO_Pin_5 | GPIO_Pin_3;   /* sets A6, A5 and A3 */
GPIOA->BSRRH = GPIO_Pin_6 | GPIO_Pin_5 | GPIO_Pin_3;   /* clears A6, A5 and A3 */

This is the best solution so far in this thread IMO.
Clearly readable which pins are being set and reset here.
Just turn this into two inline functions that takes the gpio port and then the pin bitmask and hey presto: super readable and maintainable code with no overhead
 

Offline ajb

  • Super Contributor
  • ***
  • Posts: 2582
  • Country: us
Re: BSRR in STM32F4xx.h
« Reply #12 on: April 08, 2019, 07:35:38 pm »
So instead of
GPIOA->BSRRL = 0x01101000;
GPIOA->BSRRH = 0x01101000;   // toggles A6,A5,A3

use please
GPIOA->BSRRL = GPIO_Pin_6 | GPIO_Pin_5 | GPIO_Pin_3;   /* sets A6, A5 and A3 */
GPIOA->BSRRH = GPIO_Pin_6 | GPIO_Pin_5 | GPIO_Pin_3;   /* clears A6, A5 and A3 */

The second is hardly any better than the first in practice.  It still doesn't tell me what the logical function of the pin(s) you're toggling are, just which physical pins they're mapped to, which I don't really care about when I'm writing software.  So I have to mentally know that pin A3 is the first red LED or whatever.  And what happens when you revise the board layout and the function that was on A3 is now on B13? 

Better to define, in one place, how logical signals map to your IO pins, and then use those logical names throughout your code.  I tend to do the same thing with peripherals, where I have multiples on a chip.  That way the code explicitly says which logical function I'm interacting with, which makes it easier to spot errors, and if I revise the circuitry I only have to edit one file to correct the pin mappings.  It doesn't really matter what you use in that definition, as long as it's clear to the future reader.

Code: [Select]
#define DEBUG_RX_I E,0
#define DEBUG_TX_O E,1
#define DEBUG_RX_AF 8
#define DEBUG_TX_AF 8
#define DEBUG_USART UART8
#define DEBUG_USART_IRQN UART8_IRQn
#define DEBUG_USART_IRQHANDLER UART8_IRQHandler
// Panel IO
#define BTN_DOWN_I B,2
#define BTN_MENU_I F,11
#define BTN_LEFT_I F,12
#define BTN_ENTER_I F,13
#define BTN_RIGHT_I F,14

Obviously you could also do this abstraction through functions and variables as opposed to macros, which would give you runtime flexibility at the expense of more complex code that may or may not optimize out depending on how you implement them.
 

Offline Yansi

  • Super Contributor
  • ***
  • Posts: 3893
  • Country: 00
  • STM32, STM8, AVR, 8051
Re: BSRR in STM32F4xx.h
« Reply #13 on: April 08, 2019, 07:59:47 pm »
It is not about the logical function, but READABILITY of the code. Nobody is likely interested in solving cryptic 16 or eve 32bit magical constants throughout the code.
 

Offline Siwastaja

  • Super Contributor
  • ***
  • Posts: 8105
  • Country: fi
Re: BSRR in STM32F4xx.h
« Reply #14 on: April 08, 2019, 08:53:26 pm »
The whole point of the BSRR register is that you can atomically and efficiently both set and reset any arbitrary bit combination at the same time; separating them into two 16-bit accessors is a mistake. I haven't seen it on STM32 headers before, even on older versions, pretty weird. I suggest you use the newer version or fix it manually to your header file, also for compatibility reasons.

About the readability. People tend to like rectifying unreadable mess of the others (with good intentions, of course), but sadly, I feel like while doing so, we lack some ambition, I mean, the "better" way tends to be OK...ish, but not much more. We could certainly do better than just always blindly use the "good old" microcontroller header way of:

Code: [Select]
SOME_REGISTER = SOME_REGISTER_SOMETHING_1 | SOME_REGISTER_MOAR_THINGS2 | SOME_REGISTER_HEY_WE_LIKE_DUPLICATION | SOME_REGISTER_ASDFG.

This is a strange mixture of "abstraction" and total non-abstraction (only duplication) and I don't like it. So sometimes, I just feel very... hacky, and go with:
Code: [Select]
SUPER_PERIPHERAL = 0b10100010;
On the surface, it looks unwritable and unreadable, the intent doesn't show, but the official alternative often offered doesn't show the intent any better:

Code: [Select]
SUPER_PERIPHERAL = SUPER_PERIPHERAL_ASDFG | SUPER_PERIPHERAL_QWEHJB | SUPER_PERIPHERAL_KLMAMF;

The "library way" is even worse:
Code: [Select]
struct Super_Peripheral_Init_Struct letsGiveAsLongNameAsPossibleInitStruct;
memset(letsGiveAsLongNameAsPossibleInitStruct, 0, sizeof(struct Super_Peripheral_Init_Struct));
letsGiveAsLongNameAsPossibleInitStruct.configureTheASDFGFeature = SUPER_PERIPHERAL_INIT_STRUCT_CONFIGURE_THE_ASDFG_FEATURE_ON; // Don't abstract anything
letsGiveAsLongNameAsPossibleInitStruct.configureTheQWEHJBFeature = SUPER_PERIPHERAL_INIT_STRUCT_CONFIGURE_THE_QWEHJB_FEATURE_ON; // Expose everything
letsGiveAsLongNameAsPossibleInitStruct.configureTheKLMAMFFeature = SUPER_PERIPHERAL_INIT_STRUCT_CONFIGURE_THE_KLMAMF_FEATURE_ON; // Copy paste a lot
InitTheSuperPeripheral(Super_Peripheral1, &letsGiveAsLongNameAsPossibleInitStruct); // Just writes the registers. No real abstraction done.

Oh my god, let's forget about that.

In reality, in most cases when accessing MCU peripherals, you absolutely need to have the register description datasheet page open for full descriptions both when writing your code, and when reading through it (to get the actual full meaning), so it doesn't make a difference in the end whether you use bit numbers or the totally gibberish (unless you have the datasheet open!) defined names. So the readability is zero anyway, and writability is worse when you need to duplicate and worse, lookup the header files to get all the masks you need.


So I started doing this instead: I don't use the names since they are not descriptive enough; but I add comments to the bit indeces to tell the reader what's actually going on:

Code: [Select]
SUPER_PERIPHERAL =
    1<<7 /*Bit called ASDFG, actually turns on the output register carburetor*/ |
    1<<5 /*Enable transmitter (called QWEHJB in the datasheet for unknown reasons)*/ |
    1<<1 /*STM32 trap: the mysterious, almost unspecified KLMAMF bit MUST BE TURNED ON, otherwise the multiplexer cam shaft breaks randomly*/;

If someone still has problems with this, their loss. I have done my best, and it's definitely better than the status quo. (I also comment in the document number for the reference manual, and often refer to the actual pages.)


For the GPIO, I personally use these macros:
Code: [Select]
#define HI(port, idx) do{(port)->BSRR = 1UL<<(idx);}while(0)
#define LO(port, idx) do{(port)->BSRR = 1UL<<(16+idx);}while(0)

(Of course, when I need the atomicity feature, I bypass these macros and write to the BSRR manually.)

I like both the writability and the readability of
Code: [Select]
HI(PORTB, 12);
Which doesn't include much extra that isn't in your head, and in the documentation (everything says PB12).

Then abstracting the actual pin meanings doesn't involve replicating stupid, mixed-case, long header/vendor specific names like "GPIO_Pin_3" (or was it GPIO_pin_3, or GPIO_Pin3?). Instead, I define:
Code: [Select]
#define IMU024_G_NCS_PORT GPIOA
#define IMU024_G_NCS_PIN 14

(Some more preprocessor trickery could help getting rid of duplication of the functionality name; the optimal case would be, with no duplication, and to match with schematics and device pinout documentation:
Code: [Select]
#define IMU024_G_NCS PA14
...
        LO(IMU024_G_NCS);
But I'm not there yet  :-\. (ajb is very close in his post above.)


Sometimes, if I feel like it, I abstact the complete register operation to a macro:
Code: [Select]
#define en_gate_pha() do{ HI(GPIOG, 1); }while(0)
#define dis_gate_pha() do{ LO(GPIOG, 1); }while(0)

Using "GPIO_Pin_3" here would do nothing else than introduce extra typing, and introduce another level of arbitrary per-product line header dependency, for no reason.

I agree that "direct" magic numbers are not that neat, which is why I went for some helper macros. Some of these look ugly, but are write-once, use for years, and make the application code look nice, for example:

Code: [Select]
#define IO_SET_ALTFUNC(port, pin, af) do{ _Pragma("GCC diagnostic push") _Pragma("GCC diagnostic ignored \"-Wshift-count-negative\"")  _Pragma("GCC diagnostic ignored \"-Wshift-count-overflow\"") \
if((pin)<8) {uint32_t _tmp_ = (port)->AFR[0]; _tmp_ &= ~(0b1111UL<<((pin)*4));     _tmp_ |= af<<((pin)*4);     (port)->AFR[0] = _tmp_;} \
        else {uint32_t _tmp_ = (port)->AFR[1]; _tmp_ &= ~(0b1111UL<<(((pin)-8)*4)); _tmp_ |= af<<(((pin)-8)*4); (port)->AFR[1] = _tmp_;} _Pragma("GCC diagnostic pop") }while(0)

Or,
Code: [Select]
#define DMA_CLEAR_INTFLAGS(_dma_, _stream_) do{                   \
if     ((_stream_) == 0) (_dma_)->LIFCR = 0b111101UL<<0;  \
else if((_stream_) == 1) (_dma_)->LIFCR = 0b111101UL<<6;  \
else if((_stream_) == 2) (_dma_)->LIFCR = 0b111101UL<<16; \
else if((_stream_) == 3) (_dma_)->LIFCR = 0b111101UL<<22; \
else if((_stream_) == 4) (_dma_)->HIFCR = 0b111101UL<<0;  \
else if((_stream_) == 5) (_dma_)->HIFCR = 0b111101UL<<6;  \
else if((_stream_) == 6) (_dma_)->HIFCR = 0b111101UL<<16; \
else if((_stream_) == 7) (_dma_)->HIFCR = 0b111101UL<<22; \
}while(0)

Hope you get the point. Sorry for the incoherent message, hope it makes any sense.

But I do admit that there is a lot of "matter of taste" going on here.
« Last Edit: April 08, 2019, 08:57:21 pm by Siwastaja »
 
The following users thanked this post: newbrain

Offline Yansi

  • Super Contributor
  • ***
  • Posts: 3893
  • Country: 00
  • STM32, STM8, AVR, 8051
Re: BSRR in STM32F4xx.h
« Reply #15 on: April 08, 2019, 09:27:24 pm »
You seem to be a tad annoyed by longer (although very clear) names, don't you? :P

Get a better IDE. Noone writes these names in full by hand anyway, so whats the point?  :P
 

Offline Siwastaja

  • Super Contributor
  • ***
  • Posts: 8105
  • Country: fi
Re: BSRR in STM32F4xx.h
« Reply #16 on: April 08, 2019, 09:55:50 pm »
You seem to be a tad annoyed by longer (although very clear) names, don't you? :P

Missed the point entirely. The problem is when the names are NOT clear, or are not providing any extra information. Only if/when this is the case, being lengthy becomes really annoying. OTOH, you can accept such minor issue when it makes sense.

Quote
Get a better IDE. Noone writes these names in full by hand anyway, so whats the point?  :P

I strongly disagree with the idea that the code should be obfuscated, and then this obfuscation process automated. Even if you automate the typing, the readability still suffers.

Especially, the peripheral bit mask names tend to be really gibberish, often with one letter differences, like ASDFG and ADSFG. If you speed up your writing with an IDE autocomplete, you are bound to make a mistake at some point. During 20 years, I have learned that better just have the datasheet open and go through the register, very carefully, bit by bit, and comment what you are doing. Takes 5 minutes - saves days of debugging. (I used IDEs for microcontroller work for ca. 1998 - 2010. Now it's text editor, make + custom tools per project.)

Of course I want to be practical instead of pedantic - sometimes this automation is inevitable (for example, I do write VHDL with assisting tools thanks to some braindeadness in the VHDL syntax), but in the case of embedded C, there is very little reason to use such amount of copypasting or autogeneration as seen today (the worst offenders are the now discontinued (thank god) STM32 peripheral libraries).

My productivity increases every time I drop unnecessary tools and unnecessary layers of complexity (or obfuscation). Similarly, my productivity increases whenever I create my own tools, where actually needed.

Yes, I know "obfuscation" is a bit strong here. It's not such a big deal, but in principle, it's still obfuscation. In any case:

The IO port is called PA7 in the datasheet, in your schematics, through the whole process. It should be as close as PA7 as possible in the code. GPIO_Pin_7 is only obfuscation, nothing else. Yes, text editor automation makes it behave better, but I have very hard time understanding why would you want to do that, when you can easily solve the problem completely, instead of bodging around it?

Similarly, for example, you may have a chapter about "enabling TX" in the manual, which may involve setting a bit called, say, TXE, in CTRL_REG.

enable_tx();
is a real abstraction.

CTRL_REG |= CTRL_REG_TXE;
is pure laziness by design. It's tradition dating back to at least 90's. I'm not saying it's bad. It's just... not good either. It's status quo, it's acceptable, it's neutral. But:

CTRL_REG |= 1;
is equally good or bad, IMHO.

Both absolutely require comments (the first one could look like "clearing some TX empty flag" as well!), and both require that you look at the actual datasheet every time this code line is relevant. I don't see much difference in readability. I think this is a fairly minor issue in any case; the real readability issues will be elsewhere (especially, how all the data and program flow is organized). And, copy-paste/autogeneration mentality is a real showstopper there. Not saying you couldn't switch mentalities as per required, but... There's always a risk, and it's good to understand it.
« Last Edit: April 08, 2019, 10:10:55 pm by Siwastaja »
 

Offline lucazader

  • Regular Contributor
  • *
  • Posts: 221
  • Country: au
Re: BSRR in STM32F4xx.h
« Reply #17 on: April 08, 2019, 10:10:23 pm »
For the GPIO, I personally use these macros:
Code: [Select]
#define HI(port, idx) do{(port)->BSRR = 1UL<<(idx);}while(0)
#define LO(port, idx) do{(port)->BSRR = 1UL<<(16+idx);}while(0)

Is there any reason you don't use an inline function here instead of the macro?
 

Offline Siwastaja

  • Super Contributor
  • ***
  • Posts: 8105
  • Country: fi
Re: BSRR in STM32F4xx.h
« Reply #18 on: April 08, 2019, 10:19:13 pm »
For the GPIO, I personally use these macros:
Code: [Select]
#define HI(port, idx) do{(port)->BSRR = 1UL<<(idx);}while(0)
#define LO(port, idx) do{(port)->BSRR = 1UL<<(16+idx);}while(0)

Is there any reason you don't use an inline function here instead of the macro?

I have had some really bad experiences with inline functions, and I was just being careful. I'm almost 100% sure this one could be replaced no problem. In 99% of cases, they work as intended.

The problem is, when GCC fails inlining an inline function, it's not called a compiler bug, so these are not high-priority issues. After all, the viewpoint they are coming from (PC, mainly userland code), is that this is a very slight performance/optimization issue. But on a microcontroller code, it can be a matter of functioning at all.

IIRC, in this specific header, I had one function that failed to inline no matter what I did, so I made them all macros. Later, I learned about __attribute__((always_inline)), but quite frankly, using a macro is a more portable way to say the same.

Always remember to use -Winline with GCC when you inline anything where inlining does matter.

As a general note macro vs. inline function, they offer the opposite set of advantages / disadvantages: macro doesn't type check, but allows type genericity; function does type check, but doesn't allow type genericity (in C++, you can of course overload or use templates, which offers both advantages).
 

Offline lucazader

  • Regular Contributor
  • *
  • Posts: 221
  • Country: au
Re: BSRR in STM32F4xx.h
« Reply #19 on: April 08, 2019, 10:40:29 pm »
Thanks for the clarification and succinct reasoning!
(not being facetious here, your answer is great)
 

Offline Yansi

  • Super Contributor
  • ***
  • Posts: 3893
  • Country: 00
  • STM32, STM8, AVR, 8051
Re: BSRR in STM32F4xx.h
« Reply #20 on: April 08, 2019, 10:54:31 pm »
enable_tx();
is a real abstraction.

CTRL_REG |= CTRL_REG_TXE;
is pure laziness by design. It's tradition dating back to at least 90's. I'm not saying it's bad. It's just... not good either. It's status quo, it's acceptable, it's neutral. But:

CTRL_REG |= 1;
is equally good or bad, IMHO.

Both absolutely require comments (the first one could look like "clearing some TX empty flag" as well!), and both require that you look at the actual datasheet every time this code line is relevant. I don't see much difference in readability. I think this is a fairly minor issue in any case; the real readability issues will be elsewhere (especially, how all the data and program flow is organized). And, copy-paste/autogeneration mentality is a real showstopper there. Not saying you couldn't switch mentalities as per required, but... There's always a risk, and it's good to understand it.

As reasonable as you may sound, I just can't agree with what you are presenting here.

First abstract example - okay, nothing to say. Second example - you seem to be a typical programmer - you need to changes things and make them differently, just for the sake of it. Who cares if it is from 90's or not, it is a well readable piece of code, unlike your last example, where one would really need to open the datasheet to see, what freaking bits are 0x00040C00.  No thanks!  :palm:

If I continue to take examples, so lets stay with TXE: 
(SPI1->SR & SPI_SR_TXE)
It is more then self-explanatory even without additional comment and even without opening the datasheet. You even don't need to refer to the library manuals, as the only thing you remember, is that SPI has a SR reg with TXE (TX Empty) bit in it.  Which is quite lovely as ST has their ducks quite in a row and the register and bits are very VERY similar if not same across multiple devices or even complete product lines.

I don't understand why would you ever suggest to replace SPI_SR_TXE with some cryptic 32bit number!  It is not equally bad, it is pure dumb!
 

Offline lollandsterTopic starter

  • Contributor
  • Posts: 40
  • Country: no
Re: BSRR in STM32F4xx.h
« Reply #21 on: April 09, 2019, 06:15:15 am »
I downloaded Core F4. I find the ST webpage somewhat hard to understand and I could not find Cube anywhere under STM32F401RC, but knowing what I was looking for enabled google to help me, thanks Newbrain!
Inside STM32Cube F4 i found STM32F401xc.h which replaced my STM32F4xx.h without breaking anything. And it uses 32 bit for BSRR, so it fixes my problem. I had to keep my old core_cm4.h file though as the one I found in CubeF4 gave me some errors.

FYI Siwastaja, I do program the registers with the reference manual open and I'm using the defined names from STM32F401xc.h because it perfectly matches the names of the registers in the reference manual. So when I write RCC->CR |= RCC_CR_PLLON, I know that I can find the register under the RCC chapter and that the PLLON bit is in the CR register found in that chapter. It may not be the best way, but it is the way I'm doing it for now. I may become wiser in the future.
 

Offline Siwastaja

  • Super Contributor
  • ***
  • Posts: 8105
  • Country: fi
Re: BSRR in STM32F4xx.h
« Reply #22 on: April 09, 2019, 08:49:11 am »
I don't understand why would you ever suggest to replace SPI_SR_TXE with some cryptic 32bit number!  It is not equally bad, it is pure dumb!

Not suggesting that. I'm just saying, when working with peripheral registers, I always work very closely with the reference manual at hand, all the times I do anything with these registers (write or read the code), because I find you tend to need the full describtions instead of the acronym names, because they do have catches. It's better to do this work carefully and then abstract it away inside a simple "driver" function or a macro. This way, when reading the code, you are not trying to neither decipher the bare bit number, nor SPI_SR_TXE like macros. You read the function names, and comments, and then finally, the manual. This is why I'm saying there is not much difference in readability.

Now, generic bitmasks (like 1<<5, which says "bit number 5" to anyone experienced in embedded C) do match perfectly what I see in the refman: it lists bit number 5. It also says TXE, but it doesn't say SPI_SR_TXE. With these defines, I need to hold two sets of slightly different naming schemes in my head. Then, if I ever need to print out the actual register value in a trace or a debug system, then it's back to the raw bit indices again. So I'm avoiding "multiple names for the same thing" phenomenon here. Instead, I'm all for higher level abstraction.

The idea that a more verbose, duplicated coding style is "easier to read" is a fallacy. It feels easier to read, our brain is designed to process natural language text. Our brain also has a auto-correct system, did you spot all my typos? I guess not. It's a pattern matching, pick the closest good-looking match silently. So while a verbose copy-pasta language is fast to read out loud, your brain is not giving the correct weight to the things requiring it (SPI_SR_TXE). Thus I want to avoid duplication and unnecessary noise.

SPI_SR_TXE is also not a good example, because it's one of the simplest peripherals there. Many others have easily over 200-300 configuration and status bits, with completely gibberish acronym names, even for those who do understand these peripherals, like HRTIM_BDMUPR_MDIER. I just looked it up randomly, and even having done several complex HRTIM designs (I like it), I have no idea whatsoever what this would be. So even if SPI_SR_TXE would be a descriptive, easy to understand name for some, this concept is not scalable: the next one isn't understandable, and you need a full manual lookup anyway.

These bitmask defines should, but do not always match the datasheet names. And when they do not match, it completely ruins the whole idea.

For example, the datasheet says:
HRTIM_TIMxDIER register has bit RSTx2DE, where x = A, B, C, D or E. Following this, this code should work:
Code: [Select]
HRTIM->TIMBDIER |= HRTIM_TIMBDIER_RSTB2DE;
Or wait? Should it be:
Code: [Select]
HRTIM->TIMBDIER |= HRTIM_TIMxDIER_RSTx2DE; // because the timer units are similar and could share the bitmask defines?
Or maybe it is:
Code: [Select]
HRTIM->TIMxDIER[1] |= HRTIM_TIMxDIER_RSTx2DE;
Or maybe:
Code: [Select]
HRTIM->TIM[1].DIER |= HRTIM_TIMxDIER_RSTx2DE;


These would be logical: and it's none of these. In reality, it's:

... drum roll ...

Code: [Select]
HRTIM1->sTimerxRegs[1].TIMxDIER |= HRTIM_TIMDIER_RST2IE;

Complete illogical mess:
1) name mismatch: in the complete documentation, there is no HRTIM1 instantiation, only HRTIM
2) name mismatch: what the heck is sTimerxRegs?
And finally, most relevant to this discussion:
3) inner logic mismatch: in the regname define, it's TIMxDIER, then in the mask macro, it's just TIMDIER.

Not saying this is a huge problem, or problem at all, and not saying you should change what you do, you are doing just fine. Just pointing out that your way is not superior, either, and isn't actually making the code any more readable, and is factually making it less writable, and then you need automation to compensate. It's still cryptic code that needs to be commented and referenced with the reference manual; exactly like the bare bit numbers you so strongly dismissed.

Unlike you say, I see the variability in STM32 peripherals a big, time-consuming problem. Almost every freaking STM32 MCU I have used so far has had differences in almost all basic peripherals like USART, SPI, I2C or DMA. You could circumvent this with really good cross-compatible library abstractions, but ST hasn't been able to do that properly, and quite frankly, it is difficult, I couldn't do it either; MCU users tend to be demanding in both flexibility and performance. To be fair, maybe I have been unlucky, because I have used devices very widely from different families (F0, F2, F3, F7, H7), and different eras (F224, for example, is quite old compared to the new H7).
« Last Edit: April 09, 2019, 08:59:13 am by Siwastaja »
 

Offline MT

  • Super Contributor
  • ***
  • Posts: 1616
  • Country: aq
Re: BSRR in STM32F4xx.h
« Reply #23 on: April 09, 2019, 05:01:00 pm »
And do not modify header files unless you understand well what you are doing :).

Just recently had to modify an old F334 and F301 project to discover loads of things didnt work due to missing register defs in svd file, wrongly and missing NVIC defs in .h file and even wrongly labeled NVIC IRQs in startup .s file! 3 independent ST files all garbled up!  :palm:  ST is famous for the inconsistently inconsequently language use of SET and RESET to SET and CLEAR register bits. :horse: Why shall i be punished to spend hours on debug ST arrogant crap?
« Last Edit: April 09, 2019, 07:58:28 pm by MT »
 

Offline Yansi

  • Super Contributor
  • ***
  • Posts: 3893
  • Country: 00
  • STM32, STM8, AVR, 8051
Re: BSRR in STM32F4xx.h
« Reply #24 on: April 09, 2019, 10:16:09 pm »
Because you are arrogant as well? :P
 


Share me

Digg  Facebook  SlashDot  Delicious  Technorati  Twitter  Google  Yahoo
Smf