Fix FDCAN interrupt configuration in CO_driver_STM32.c

[SWolfSchunk]

Fix assertion by using realistic values from stm32h5xx_hal_fdcan.h

[MaJerle]

Does this compile for all series or you just tested H5? If only H5, we must verify compilation is OK for the rest, too.

[SWolfSchunk]

I can only validate H5.
Maybe it is a better way to introduce some kind of CONFIG header (in examples / as template) and include it here.
This way it makes it nescessary to modify code / copy code when used as submodule.

[MaJerle]

I am almost 100% sure this won't compile for STM32G0, or older STM32s, so I prefer we don't merge this now.

My suggestion, if you agree, is that in the same file, we start the #if-else macro logic to set the flags.

#if defined(STM32H5xx)
#define MY_VAR ....
#else
/* Keep legacy for now */
#define MY_VAR 0xFFFFFFFF
#endif

[SWolfSchunk]

Yes I agree this would be a much better approach.

[TheCitrusMan]

I have a couple G0 based projects, I may be able to check this later today...

[SWolfSchunk]

I added the check based on STM32H5xx_HAL_CONF_H which is always defined by stm32h5xx_hal_conf.h.
Otherwise some -D define or header inclusion will be necessary.
I personally use -DSTM32H573xx that would be much to specific.

[TheCitrusMan]

The macros do exist in stm32g0xx_hal_fdcan.h which means it compiles correctly.

[MaJerle]

Yes, the G0 and H5 use the same FDCAN configuration, you are right. Same will be with C5, G4, C0 and at least U5.

Anyway, the 0xFFFFFFFF was used to simplify the usage. For example STM32H7 uses FDCAN with 32 buffers, therefore in this case we want to get interrupts for all of them. Same here.

The HAL driver will correctly select bits in the implementation.

Therefore, when I think, do we need to do any update?

[SWolfSchunk]

@MaJerle as @TheCitrusMan verified one of your doubts is clear.
The other doubt is older STM32s but I think all of the already used FDCAN Makros are not available on (e. g. STM32F4 ...).
So the question is which ones should be taken care of?

[SWolfSchunk]

@MaJerle
From stm32h5xx_hal_fdcan.c:

 HAL_StatusTypeDef HAL_FDCAN_ActivateNotification(FDCAN_HandleTypeDef *hfdcan, uint32_t ActiveITs,
                                                 uint32_t BufferIndexes)
{
  HAL_FDCAN_StateTypeDef state = hfdcan->State;
  uint32_t ITs_lines_selection;

  /* Check function parameters */
  assert_param(IS_FDCAN_IT(ActiveITs));
  if ((ActiveITs & (FDCAN_IT_TX_COMPLETE | FDCAN_IT_TX_ABORT_COMPLETE)) != 0U)
  {
    assert_param(IS_FDCAN_TX_LOCATION_LIST(BufferIndexes));
  }

From stm32h5xx_hal_fdcan.h:

#define IS_FDCAN_TX_LOCATION_LIST(LOCATION) (((LOCATION) >= FDCAN_TX_BUFFER0) && \
                                             ((LOCATION) <= (FDCAN_TX_BUFFER0 | FDCAN_TX_BUFFER1 | FDCAN_TX_BUFFER2)))

If assertion is active this is a problem as IS_FDCAN_TX_LOCATION_LIST wont return true in case of 0xFFFFFFFF

[TheCitrusMan]

Yes, the G0 and H5 use the same FDCAN configuration, you are right. Same will be with C5, G4, C0 and at least U5.

Anyway, the 0xFFFFFFFF was used to simplify the usage. For example STM32H7 uses FDCAN with 32 buffers, therefore in this case we want to get interrupts for all of them. Same here.

The HAL driver will correctly select bits in the implementation.

Therefore, when I think, do we need to do any update?

I have a G4 project also -> confirm that this is also fine.

[HamedJafarzadeh]

Thanks for your contribution. I checked your changes on two targets [STM32G0 and STM32F0] and both performed ok.

[SWolfSchunk]

Well what I added was just fine for me with a STM32H5 target but maybe you should only merge the first commit as in the second I introduced a guard that exclusively works on these targets because of @MaJerle s hint:

I am almost 100% sure this won't compile for STM32G0, or older STM32s, so I prefer we don't merge this now.

My suggestion, if you agree, is that in the same file, we start the #if-else macro logic to set the flags.

#if defined(STM32H5xx)
#define MY_VAR ....
#else
/* Keep legacy for now */
#define MY_VAR 0xFFFFFFFF
#endif

[HamedJafarzadeh]

In my view, it is fixing the issue for STM32H5xx and defaults to legacy in other cases, because of #if-else marcos. So I don't see any issue with merging this. But as you mentioned this might not work across all series and might need some modifications. On my setup I could test it on F0 and G0 and it worked fine.

[MaJerle]

Passing 0xFFFFFFFF should normally fail the assertion as far as I know, if assertion is enabled.
STM32 CAN modules are normally similar at these families: H5, U3, U5, C5, G0, C0, G4.
STM32H7 has a FDCAN module that supports 32 buffers.

We could do the #if defined(FDCAN_TX_BUFFER31) (to check if it is large one for H7) and assign 0xFFFFFFFF and then do the #elif defined(FDCAN_TX_BUFFER2) which indicates the smaller FDCAN options:

• STM32H5xx
• STM32G4xx
• STM32U5xx
• ...

Finally there are products like L4, F1, F4, that do not have FDCAN but only CAN, but that’s outside this driver.

[SWolfSchunk]

@MaJerle I think your solution would be the best.

[TheCitrusMan]

Would it then not make more sense to put it like this...

#ifndef BUFFERE_INDEXES

#ifdef STM32H7xx_HAL_CONF_H
#define BUFFERE_INDEXES 0xFFFFFFFF
#else
#define BUFFERE_INDEXES FDCAN_TX_BUFFER0 | FDCAN_TX_BUFFER1 | FDCAN_TX_BUFFER2
#endif

#endif

[MaJerle]

Not being limited to the family name makes it easier in the future with the compatibility.

[SWolfSchunk]

Finally there are products like L4, F1, F4, that do not have FDCAN but only CAN, but that’s outside this driver.

Those are not affected as only FDCAN needs to be taken into account. See the snippet:

#ifdef CO_STM32_FDCAN_Driver
    if (HAL_FDCAN_ActivateNotification(((CANopenNodeSTM32*)CANptr)->CANHandle,
                                       0 | FDCAN_IT_RX_FIFO0_NEW_MESSAGE | FDCAN_IT_RX_FIFO1_NEW_MESSAGE
                                           | FDCAN_IT_TX_COMPLETE | FDCAN_IT_TX_FIFO_EMPTY | FDCAN_IT_BUS_OFF
                                           | FDCAN_IT_ARB_PROTOCOL_ERROR | FDCAN_IT_DATA_PROTOCOL_ERROR
                                           | FDCAN_IT_ERROR_PASSIVE | FDCAN_IT_ERROR_WARNING,
                                       0xFFFFFFFF )
        != HAL_OK) {
        return CO_ERROR_ILLEGAL_ARGUMENT;
    }
#else
    if (HAL_CAN_ActivateNotification(((CANopenNodeSTM32*)CANptr)->CANHandle, CAN_IT_RX_FIFO0_MSG_PENDING
                                                                                 | CAN_IT_RX_FIFO1_MSG_PENDING
                                                                                 | CAN_IT_TX_MAILBOX_EMPTY)
        != HAL_OK) {
        return CO_ERROR_ILLEGAL_ARGUMENT;
    }
#endif

I still think the solution propposed be @MaJerle would be the best way:

#ifndef BUFFER_INDEXES
#if defined (FDCAN_TX_BUFFER31)
#define BUFFER_INDEXES 0xFFFFFFFFU
#elif defined (FDCAN_TX_BUFFER2)
#define BUFFER_INDEXES FDCAN_TX_BUFFER0 | FDCAN_TX_BUFFER1 | FDCAN_TX_BUFFER2
#else
#define BUFFER_INDEXES 0xFFFFFFFFU
#warning "BUFFER_INDEXES not defined"
#endif
#endif

Maybe we should rename BUFFER_INDEXES into FDCAN_BUFFER_INDEXES for a proper Naming

[MaJerle]

Please update the proposal as You mentioned and also I propose to rename FDCAN_BUFFER_INDEXES to FDCAN_TX_BUFFER_INDEXES.

Then we merge.

[SWolfSchunk]

@MaJerle i will do it in #110