0

I'm trying to use the structure for centralizing my hardware configuration. However, this makes my codes slow later while the struct has been defined as a global variable in the RAM.

For example, I have defined.

typedef struct
{
    PeriphBus       TIM_Bus;
    uint32_t        TIM_Clk;
    uint16_t        TIM_Prescaler;
    uint32_t        TIM_CounterMode;
    uint32_t        TIM_Autoreload;
    uint32_t        TIM_ClockDivision;
    uint32_t        TIM_RepetitionCounter;
    TIM_TypeDef     *TIM_Peripheral;
    IRQn_Type       TIM_Interrupt;
    uint32_t        TIM_IPeriority;
} TIMHandler;

TIMHandler  TIMCCS = {
    .TIM_Bus                        = APB1,
    .TIM_Clk                        = LL_APB1_GRP1_PERIPH_TIM2,
    .TIM_Prescaler                  = (10000 - 1),
    .TIM_CounterMode                = LL_TIM_COUNTERMODE_UP,
    .TIM_Autoreload                 = (1000 - 1),
    .TIM_ClockDivision              = LL_TIM_CLOCKDIVISION_DIV1,
    .TIM_RepetitionCounter          = 0,
    .TIM_Peripheral                 = TIM2,
    .TIM_Interrupt                  = TIM2_IRQn,
    .TIM_IPeriority                 = 2,
};

And later in the code, I'm trying to reset the interrupt flag with this code.

void TIMCCS_IRQHandler (void)
{
    ... // some codes here are deleted to keep it simpler.
    LL_TIM_ClearFlag_UPDATE (TIMCCS->TIM_Peripheral);
}

Unfortunately, this last function to reset the interrupt flag is prolonged, while if I replace it with

LL_TIM_ClearFlag_UPDATE (TIM2);

It gets back to normal.

I'm wondering where I'm making a mistake. I'm using ARM GCC as the compiler for STM32F7 microcontrollers.

  • `TIMCCS` is a _global_ (i.e. _not_ a pointer). Have you tried: `LL_TIM_ClearFlag_UPDATE((&TIMCCS)->TIM_Peripheral);`? Or, better yet: `LL_TIM_ClearFlag_UPDATE(TIMCCS.TIM_Peripheral);` – Craig Estey Dec 01 '20 at 02:39
  • TIMCCS is defined earlier in the code as I put it. Not inside a function, so it's global. LL_TIM_ClearFlag_UPDATE(TIMCCS.TIM_Peripheral) generates an error (error: invalid type argument of '->' (have 'TIMHandler {aka struct }')) – Omid Ghadami Dec 01 '20 at 07:24
  • If `LL_TIM_ClearFlag_UPDATE(TIM2)` works, `LL_TIM_ClearFlag_UPDATE(TIMCCS.TIM_Peripheral)` _must_ work also because you're initializing `TIMCCS.TIM_Peripheral` to `TIM2`. What is `TIM2`? And, what is `LL_TIM_ClearFlag_UPDATE`? Because, I note that `TIM_Peripheral` is the only _pointer_ in the struct (i.e. perhaps leave off the `*`?) and it needs to be of the correct type. Specifically, what is the type for the argument to `LL_TIM_ClearFlag_UPDATE`? – Craig Estey Dec 01 '20 at 08:02
  • I think it's not a type issue, it's how the memory mapping and addressing works. Precisely the "LL_TIM_ClearFlag_UPDATE (&TIMCCS->TIM_Peripheral)" works but with an excessive delay which is not acceptable to my case. I mean "LL_TIM_ClearFlag_UPDATE (TIM2)" is faster and I was expecting to find a way to generate the same assembly code for both cases. – Omid Ghadami Dec 02 '20 at 04:18

2 Answers2

0

My first bit of advice is to review the assembly code produced between the two versions of code. Is it doing what you think it is?

Next question, do you have the optimizer turned on?

To me, at first glance it makes sense that one runs slower than the other. If you look at your first version, the ISR needs to load the address of the global struct. Then it needs to dereference that address to get the value stored in the TIM_Peripheral variable. Then it passes that value to the ClearFlag function. Whereas the second version just directly passes that value in, skipping all the memory accesses. But again, the assembly code is your friend.

  • I mean the assembly is not exactly the same but to me, it's just looked like different indirect register addressing. The optimizer is completely off (-O0) as I have a bad experience with it in codes with interrupt routines. Does optimizer solve these issues? I agree that your comment should be exactly the case; however, the code needs to be very structured and I prefer to have its configuration centralized. Is there any method to educate the compiler to put these? I mean other than defining thousands of macros? – Omid Ghadami Dec 01 '20 at 07:30
  • You are in an ISR, so you may actually care about different memory accesses, as well as in which memory this data is stored period if you have different memory types. On chip SRAM is faster than cache, which is faster than external RAM (typically). Are you at a point where you need to be counting cycles each instruction takes, because maybe the extra indirection is adding latency? Regarding macros, if you ever dig into a manufacturer's header files, that's usually what you'll find for much of the HW specific implementation. So while it's not elegant, it does get the job done. – Michael Becker Dec 02 '20 at 03:19
  • I'm using this microcontroller timer as a TDC. So what really matters to me is how long it takes till my first command executes (which is timer counter stop). What I faced however was that I have an extra delay there because of the way that I address the timer counter to stop. It's a little bit confusing as the timer is clocked at 25MHz while the core has 200MHz so a couple of instructions must not affect me which looks like it's the case. For macros, I wish there was a better way. – Omid Ghadami Dec 02 '20 at 04:15
0

This is prefaced by the top comments ...

I think it's not a type issue, it's how the memory mapping and addressing works.

No, it is a type issue. If the type passed to the function had been correct, the issue would have been resolved. Read on for the solution ...

Precisely the "LL_TIM_ClearFlag_UPDATE(&TIMCCS->TIM_Peripheral)" works but with an excessive delay which is not acceptable to my case.

Then, in other words, it does not work. See below for the full analysis why.

I mean "LL_TIM_ClearFlag_UPDATE(TIM2)" is faster and I was expecting to find a way to generate the same assembly code for both cases.

That's because this code is correct. Your code using TIMCCS is not equivalent [and is wrong].

Here is the function definition:

/**
  * @brief  Clear the update interrupt flag (UIF).
  * @rmtoll SR           UIF           LL_TIM_ClearFlag_UPDATE
  * @param  TIMx Timer instance
  * @retval None
  */
__STATIC_INLINE void LL_TIM_ClearFlag_UPDATE(TIM_TypeDef * TIMx)
{
    WRITE_REG(TIMx->SR, ~(TIM_SR_UIF));
}

The argument must be a pointer to a TIM_TypeDef struct. Obviously, TIM2 is such a pointer [because it is compiling and is working].

Within that struct, the SR element is a uint16_t that is a port address. The code clears the TIM_SR_UIF bit in the register that SR designates.

When you pass in as the argument: &TIMCCS->TIM_Peripheral you are not passing a TIM_TypeDef * pointer.

You are passing the address of your TIMCSS struct itself and not what TIM_Peripheral points to.

So, you're passing a garbage pointer value to the function. So, the function will index past the end of your struct and pick up whatever 16 bit value happens to be at the offset for SR.

This has two [bad] effects:

(1) The function will clear a bit at some random location in the register port space. This could cause some serious damage, depending on what port address is actually used. Since the processor didn't croak, it has some subtler effect.

(2) Because the port address is wrong, it will not actually clear the interrupt flag in the correct interrupt controller port!

So, when you return from the ISR, the interrupt (having not been cleared), will immediately reinterrupt the processor.

You will [again] enter the ISR and [again] not clear the correct interrupt bit. And, when the ISR returns, the processor will be immediately reinterrupted.

This cycle will continue to repeat.

So, you are being hammered with [infinitely] many spurious interrupts!

The base level code [that is desperately trying to make progress] might get a single instruction executed before the interrupt occurs again. So, it will make some [grindingly slow] progress.

So, for every base level instruction that executes, there are [probably] 10 [or more] instructions that need to be executed to enter/exit the ISR.

So, the result is that the process will seem slow (10x to 100x slow).

Once again, that's because your call to the function is incorrect.

As I said, what you want to pass to the function is the contents of the TIM_Peripheral member of your TIMCSS struct. Remember this important point: You set the value of TIM2 into that member as an initializer. And, we know that TIM2 is the correct value.

To use your struct instance correctly, you want:

LL_TIM_ClearFlag_UPDATE(TIMCCS.TIM_Peripheral);

Side notes:

This code snippet should seem familiar because I gave it in my top comments, based on what made correct c code.

Instead of answering the questions I posed [which would have helped you resolve the issue], you chose to "fixate" on your analysis, which I mentioned in the top comments had to be wrong [and gave the reason why].

In fact, my very first comment gave the correct solution.

Craig Estey
  • 30,627
  • 4
  • 24
  • 48
  • Thanks for your time and consideration. I think your point is valid and that code which I first send it here has this bug, but I know what is the error. My problem is that I'm highly sensitive to every command that I'm executing as it delays the interrupt routine. So though it's doable this way, the instruction overhead stops me from using it. I was looking to see if there is a way to have both sides; i.e. keep the code clean and address all of the modules based on a central file and having the compiler replace them with minimal instruction. – Omid Ghadami Dec 04 '20 at 03:21