nf-interpreter

nf-interpreter copied to clipboard

Description

This commit switches the define for the HAL_Time_CurrrentSysTicks from osKernelSysTicks to a new function HAL_Time_ExtendedCurrentSysTicks. As the old function osKernelSysTicks returns only a 32-bit value, the new function supports wraparound and returns a 64-bit timer value.

Motivation and Context

In a long-term test of the nanoframework, we found that the CLR does not work correctly anymore after 49 days. This is caused by the 32-bit systick value: 2^32 / 1kHz / 3600 / 24 = 49,7 days If the timeout for sleeping threads is set directly before the wraparound of the 32-bit systick counter, the threads will never wakeup again.

How Has This Been Tested?

This has been tested on a custom STM32F427 based mcu board. We wrote a simple application with two threads which both just do a simple blink pattern on separate LEDs. The one thread was programmed to use a counter, the other one used the thread.sleep() method. After provoking a wrap-around of the sysTick value (either by using only 16 bit or adding an offset), we could see that the one thread using the thread.sleep() method did not wakeup anymore.

Types of changes

• [ ] Improvement (non-breaking change that improves a feature, code or algorithm)
• [x] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Config and build (change in the configuration and build system, has no impact on code or features)
• [ ] Dependencies (update dependencies and changes associated, has no impact on code or features)
• [ ] Unit Tests (work on Unit Tests, has no impact on code or features)

Checklist:

• [x] My code follows the code style of this project.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [x] I have read the CONTRIBUTING document.
• [ ] I have added tests to cover my changes.
• [ ] All new and existing tests passed.

@martin-kuhn there are issues with the code style on the source files. A PR was submitted with the code style fixes. Please click https://github.com/CSAEngineeringAG/2g-nf-interpreter/pull/6, review the changes if you want and merge it.

Make sure to follow the project code style. Check the details here on how it works and the tools required to help you with that.

Yes, this is probably the same with all platforms using a 32 bits value.

Actually, the implementation of HAL_Time_ExtendedCurrentSysTicks is only applicable for ChibiOS, because it uses the chVTGetSystemTimeX() function directly. But maybe this could also be done with a platform-specific define.

Actually, the implementation of HAL_Time_ExtendedCurrentSysTicks is only applicable for ChibiOS, because it uses the chVTGetSystemTimeX() function directly.

Yes, that's why I was suggesting to "just" rename it and make it the HAL_Time_CurrentSysTicks 😉

But maybe this could also be done with a platform-specific define.

If we move away from define (as I suggested) we "force" each platform to have to provide it's own implementation.

It might be worth checking this has not changed in the latest chibios 21.6 release. I did see something around using 64 bit times when converting (but might well be wrong)

It might be worth checking this has not changed in the latest chibios 21.6 release. I did see something around using 64 bit times when converting (but might well be wrong)

I just checked in the latest chibiOS release. Systime_t is depending on the CH_CFG_ST_RESOLUTION which is in our case set to 32. So systime_t is uint32_t and the chVTGetSystemTimeX() function returns a 32-bit value.

@josesimoes So should I do that? Rename the function and remove the define?

@josesimoes So should I do that? Rename the function and remove the define?

Yes, please!

@josesimoes Ok! Another question: as the CH_CFG_ST_FREQUENCY is only used with 1kHz and 10kHz, there would be a much simpler implementation of the HAL_Time_SysTicksToTime() function. For 1kHz it's only sysTicks * 10000 and for 10kHz it's sysTicks * 1000. We are using this implementation at the moment. Any thoughts?

Another question: as the CH_CFG_ST_FREQUENCY is only used with 1kHz and 10kHz, there would be a much simpler implementation of the HAL_Time_SysTicksToTime() function. For 1kHz it's only sysTicks * 10000 and for 10kHz it's sysTicks * 1000. We are using this implementation at the moment. Any thoughts?

I don't understand what exactly you are suggesting... 😝

I would suggest to replace the function HAL_Time_SysTicksToTime, which is pretty complicated with 2 multiplications and a one division, with the simpler implementation I mentioned before. I guess this would make sense as we are only using 10kHz and 1kHz as systick frequency anyway.

I would suggest to replace the function HAL_Time_SysTicksToTime, which is pretty complicated with 2 multiplications and a one division, with the simpler implementation I mentioned before. I guess this would make sense as we are only using 10kHz and 1kHz as systick frequency anyway.

Got you! Then it has to be sysTicks * CH_CFG_ST_FREQUENCY to acount for the different settings that are at target level. Correct?

@martin-kuhn for consistency, could you please remove the define of HAL_Time_CurrentSysTicks on the other platforms and replace it with a call to the appropriate function? If you're comfortable with that, let me know.

actually, it's not that simple. We are using a define-based implementation like that:

static inline uint64_t HAL_Time_SysTicks1kHzToTime(uint64_t sysTicks) { #if defined(CH_CFG_ST_FREQUENCY) && (CH_CFG_ST_FREQUENCY == 1000) #define SYS_TICKS_TO_100_NANO_SECONDS 10000 #elif defined(CH_CFG_ST_FREQUENCY) && (CH_CFG_ST_FREQUENCY == 10000) #define SYS_TICKS_TO_100_NANO_SECONDS 1000 #else #error "Systick frequency must either be 1kHz or 10kHz" #endif return sysTicks * SYS_TICKS_TO_100_NANO_SECONDS; }

@martin-kuhn for consistency, could you please remove the define of HAL_Time_CurrentSysTicks on the other platforms and replace it with a call to the appropriate function? If you're comfortable with that, let me know.

yes, let me check.

actually, it's not that simple. We are using a define-based implementation like that:

static inline uint64_t HAL_Time_SysTicks1kHzToTime(uint64_t sysTicks) { #if defined(CH_CFG_ST_FREQUENCY) && (CH_CFG_ST_FREQUENCY == 1000) #define SYS_TICKS_TO_100_NANO_SECONDS 10000 #elif defined(CH_CFG_ST_FREQUENCY) && (CH_CFG_ST_FREQUENCY == 10000) #define SYS_TICKS_TO_100_NANO_SECONDS 1000 #else #error "Systick frequency must either be 1kHz or 10kHz" #endif return sysTicks * SYS_TICKS_TO_100_NANO_SECONDS; }

1. Those compiler checks seem perfectly reasonable and wise. 👍🏻
2. They can (should) be moved to the ChibiOS HAL_Time_SysTicksToTime ↪️
3. No need to have an inline function. That will colide with the global declaration or force the other platforms to inline it too. If that's a worthy improvement the compiler will consider it depending on the optimization level. 😉

@martin-kuhn there are issues with the code style on the source files. A PR was submitted with the code style fixes. Please click https://github.com/CSAEngineeringAG/2g-nf-interpreter/pull/7, review the changes if you want and merge it.

Make sure to follow the project code style. Check the details here on how it works and the tools required to help you with that.

This is somewhat related with nanoframework/Home#112, that's been there for quite some time! Because it's being called constantly it needs to be coded as lightly as possible, otherwise it introduces a severe performance penalty. We need to, perhaps, address this at another level and work on the changes pointed by that issue. It implicates deep design changes in the time base and (possibly) events...

@martin-kuhn to let you know that this is not forgotten. Just trying to organize things upwards to figure out the best way to have this change implemented.

When considering the changes, I would suggest that simple methods like the following are made inline if possible, especially if you review the generated assembler. May find little to no additional space needed for inline vs call I would expect it's much faster to execute the inline instructions out of ICache than break the flow to call a method.

// Converts CMSIS sysTicks to .NET ticks (100 nanoseconds)
uint64_t HAL_Time_SysTicksToTime(uint64_t sysTicks)
{
    // convert to microseconds from CMSIS SysTicks
    // this is a rewrite of ChibiOS TIME_I2US(n) macro because it will overflow if doing the math using uint32_t
    // need to convert from microseconds to 100 nanoseconds
    return (((sysTicks * (uint64_t)1000000) + (int64_t)CH_CFG_ST_FREQUENCY - 1) / (int64_t)CH_CFG_ST_FREQUENCY) * 10;
}

Superseded by #2515.

Hey @martin-kuhn any chance you can ping me on Discord? 😉