Implement HAL_Time_TicksToTimeMilliSec #145

josesimoes · 2017-03-02T13:36:56Z

rename function to HAL_Time_TicksToTimeMilliSec to clarify output
changed the output from 'ticks' (100ns intervals) to milliseconds to follow CMSIS API and because having a time base granularity of 1 millisecond is more than enough (100ns intervals is really too much for an embedded system) this also allows an extra efficiency of using 32bits vars and math for this instead of 64bits
update code accordingly where required
add ChibiOS implementation

Signed-off-by: José Simões jose.simoes@eclo.solutions

- rename function to HAL_Time_TicksToTimeMicroSec to clarify output - changed the output from 'ticks' (100ns intervals) to miliseconds to follow CMSIS API and because having a time base granularity of 1 milisecond is more than enough (100ns intervals is really too much for an embedded system) this also allows an extra efficiency of using 32bits vars and math for this instead of 64bits - update code accordingly where required - add ChibiOS implementation Signed-off-by: José Simões <jose.simoes@eclo.solutions>

nfbot · 2017-03-02T13:36:59Z

Hi @josesimoes,

I'm nanoFramework bot.
Thank you for your contribution!

Everything seems to be in order.
A human will be reviewing it shortly. 😉

cw2 · 2017-03-02T14:08:06Z

Sorry, but I don't understand the reason for change and what is the difference. 100ns ticks has nothing to do with system timer granularity.

josesimoes · 2017-03-02T14:29:03Z

Time base (native target) is provided by CMSIS osKernelSysTick() and returns an uint32_t.
In CMSIS the timer frequency setting for sysTick is specified in milliseconds (not nanoseconds).
Don't see a reason why nanoFramework needs a time base with a resolution higher than 1 millisecond.

If we are using this as the time base and the value is 32 bits I don't see a point on artificially 'extending' this to 64 bits just because that was used in NETMF (and in NETMF it's there with 64bits probably because that's what is used in FILETIME ).

My point being:

for it's intended use a resolution of 1 millisecond should be more than enough
using 32 bits is surely more efficient and resource wise (when compared with 64 bits)

cw2 · 2017-03-02T14:47:53Z

https://github.com/ARM-software/CMSIS/issues/44

cw2 · 2017-03-02T14:57:14Z

Ok, for timeouts (which is the actual use), milliseconds should be enough. Use osKernelSysTick() directly

auto tick = osKernelSysTick();
do {
...
} while ((osKernelSysTick() - tick) < osKernelSysTickMicroSec(100));

or wrap the timeout call similarly to ChibiOS' chTimeElapsedSince(...).

josesimoes · 2017-03-02T15:02:21Z

ARM-software/CMSIS#44

Nothing against that. And I do understand it perfectly.
But... here at nanoFramework ... as you've just pointed out this is only used for timeouts so relying on osKernelSysTick() and having it as a uint32_t is quite enough.

josesimoes · 2017-03-02T15:05:15Z

Ok, for timeouts ...........

This you showing an usage example or you are suggesting that this code block should be replacing something?

cw2 · 2017-03-02T15:07:43Z

Only usage example (how to implement a timeout check).

cw2 · 2017-03-02T13:55:21Z

src/CLR/WireProtocol/WireProtocol.cpp

@@ -273,7 +273,7 @@ bool WP_Message::Process()
                // If the time between consecutive payload bytes exceeds the timeout threshold then assume that
                // the rest of the payload is not coming. Reinitialize to synch on the next header.

-                // FIXME: if(HAL_Time_TicksToTime(curTicks - m_payloadTicks) < (UINT64)c_PayloadTimeout)
+                // FIXME: if(HAL_Time_TicksToTimeMicroSec(curTicks - m_payloadTicks) < (UINT64)c_PayloadTimeout)


I don't see where c_PayloadTimeout was updated (scaled) to retain the behavior?

Done! And I've took the opportunity to uncomment the code lines handling the timeout.

cw2 · 2017-03-02T14:06:55Z

src/CLR/Core/GarbageCollector.cpp

@@ -171,7 +171,7 @@ CLR_UINT32 CLR_RT_GarbageCollector::ExecuteGarbageCollection()
 #if defined(NANOCLR_TRACE_MEMORY_STATS)
    if(s_CLR_RT_fTrace_MemoryStats >= c_CLR_RT_Trace_Info)
    {
-        int milliSec = ((int)::HAL_Time_TicksToTime( HAL_Time_CurrentTicks() - stats_start ) + TIME_CONVERSION__TICKUNITS - 1) / TIME_CONVERSION__TICKUNITS;
+        int milliSec = (int)::HAL_Time_TicksToTimeMicroSec( HAL_Time_CurrentTicks() - stats_start );


milliSec = microSec?? There is missing scaling (division).

... or the variable name is wrong (misleading).

Corrected calculation to show milliseconds. Should remain that way in order to show a meaningful value.

cw2 · 2017-03-02T14:14:42Z

targets/os/win32/nanoCLR/Time.cpp

@@ -22,12 +22,13 @@ UINT64 HAL_Time_CurrentTicks()
    return 0; // UNDONE: FIXME: EmulatorNative::GetITimeDriver()->CurrentTicks();
 }

-INT64 HAL_Time_TicksToTime( UINT64 Ticks )
+INT32 HAL_Time_TicksToTimeMicroSec( UINT32 Ticks )
 {
    _ASSERTE(Ticks <= 0x7FFFFFFFFFFFFFFF);


It seems the value has not been updated to UINT32? This assert is now always true, which was not the original behavior.

Corrected by scaling the value.

cw2 · 2017-03-02T14:20:54Z

targets/os/win32/Include/targetHAL_Time.h

-//INT64 HAL_Time_TicksToTime(UINT64 Ticks);
-//UINT64 HAL_Time_TicksToTime();
+
+
 UINT64 HAL_Time_CurrentTicks();
 #define HAL_Time_CurrentTicks           HAL_Time_CurrentTicks


#define symbol the same? (was already there, not a new change)

cw2 · 2017-03-02T14:22:47Z

targets/os/win32/nanoCLR/Time.cpp

 {
    _ASSERTE(Ticks <= 0x7FFFFFFFFFFFFFFF);

    //No need to go to managed code just to return Time.

+	// TODO need to convert from ticks to miliseconds
    return Ticks;


So it does not really work?

josesimoes · 2017-03-02T15:23:36Z

This calls for a another adjustment. HAL_Time_CurrentTicks() is a proxy to osKernelSysTick which is uint32_t, so it should be scaled to uint32_t.
Are we in agreement on this?

cw2 · 2017-03-02T15:41:19Z

No, use osKernelSysTick() instead (either rename directly, or remove the function declaration and #define HAL_Time_CurrentTicks osKernelSysTick).

cw2 · 2017-03-02T15:41:56Z

But don't forget to find all occurrences and fix possible calculations...

josesimoes · 2017-03-02T15:44:26Z

Not seeing why the return type should remain as UINT64...
Having this with different types in native CLR and Win32 CLR code seems confusing and prone to errors...

I know that when using #define HAL_Time_CurrentTicks osKernelSysTick (which is already there) a cast will occur. But this will force the use of 64bits vars when we could be using just 32bits.

cw2 · 2017-03-02T16:01:03Z

??? Just leave it to what osKernelSysTick() has (that is exactly what #define will do). After it is changed to 64 bits, we will decide what to do - but better fix compiler errors later, than introduce nasty casting/wrapping bugs now.

- adjust return type of HAL_Time_CurrentTicks to uint32_t - update code accordingly - uncomment timeout code in WireProtocol - minor reworks in code per reviewer request Signed-off-by: José Simões <jose.simoes@eclo.solutions>

cw2 · 2017-03-02T17:24:55Z

src/CLR/Core/GarbageCollector.cpp

-        int milliSec = (int)::HAL_Time_TicksToTimeMicroSec( HAL_Time_CurrentTicks() - stats_start );
+        // convert to milliseconds to show a more meaningfull value 
+        // (because of the typical operation time showing it in microseconds could lead to a huge number...)
+        int milliSec = (::HAL_Time_TicksToTimeMicroSec( HAL_Time_CurrentTicks() - stats_start )) * 1000;


Please use a named constant or conversion macro instead of a magic value (*1000). Much easier to track down next time, when the time unit changes :)

cw2 · 2017-03-02T17:30:24Z

targets/CMSIS-OS/ChibiOS/nanoCLR/targetHAL_Time.c

 INT32 HAL_Time_TicksToTimeMicroSec(UINT32 ticks) {
    // T[s] = 1 / f[Hz] 
-    return (ticks / (osKernelSysTickFrequency * NANOHAL_TIME_CONVERSION_MILISECONDS));
+    return (ticks / (osKernelSysTickFrequency * NANOHAL_TIME_CONVERSION_MICROSECONDS));


Was this tested? For STM32F4-Discovery

#define CH_CFG_ST_FREQUENCY 10000

the multiplication overflows.

Glad you spot this... This made me realize that we don't need micro seconds calculation for timeouts because the sys clock is... milliseconds!

- as the system clock is in milliseconds it doesn't make sense to be performing the calculations in microseconds - update code accordingly - adjust target ChibiOS config to use 1ms as sys time base - adjust target ChibiOS config to use classic periodic tick (was set to use tick-less) Signed-off-by: José Simões <jose.simoes@eclo.solutions>

cw2 · 2017-03-02T20:15:12Z

targets/CMSIS-OS/ChibiOS/nanoCLR/targetHAL_Time.c

+// Converts ticks (CMSIS sysTicks) to milliseconds
+INT32 HAL_Time_TicksToTimeMilliSec(UINT32 ticks) {
+
+    return ticks * (NANOHAL_TIME_CONVERSION_MICRO_TO_SECONDS / osKernelSysTickFrequency);


How about ChibiOS' macro ST2MS() ?
http://chibios.sourceforge.net/docs3/rt/group__time.html#ga38545158d20aed90293e73a970d66b94

Looks good to me!
Nice finding.

Actually it won't work... There are calls that pass an argument which makes this not suitable. If they were all from sysTick it would, like this it won't.

I don't understand - ST2MS macro has an argument, return ST2MS(ticks); (? )

You're right. I checked it on a forum question and it looked like there was no argument... So: it fits the purpose. Let's go with this one.
Although I still have the concern that it can "overflow".

- ChibiOS macro STMS(n) will overflow when doing the math with uint32_t Signed-off-by: José Simões <jose.simoes@eclo.solutions>

josesimoes added the Area: Targets label Mar 2, 2017

josesimoes requested a review from cw2 March 2, 2017 13:36

nfbot added the DCO-check-valid label Mar 2, 2017

cw2 suggested changes Mar 2, 2017

View reviewed changes

Rework types and adjust code accordingly

5f5385a

- adjust return type of HAL_Time_CurrentTicks to uint32_t - update code accordingly - uncomment timeout code in WireProtocol - minor reworks in code per reviewer request Signed-off-by: José Simões <jose.simoes@eclo.solutions>

cw2 suggested changes Mar 2, 2017

View reviewed changes

josesimoes changed the title ~~Implement HAL_Time_TicksToTimeMicroSec~~ Implement HAL_Time_TicksToTimeMilliSec Mar 2, 2017

nfbot added the DCO-check-valid label Mar 2, 2017

cw2 suggested changes Mar 2, 2017

View reviewed changes

Replace code with ChibiOS macro rewrite and correct return type

9881e21

- ChibiOS macro STMS(n) will overflow when doing the math with uint32_t Signed-off-by: José Simões <jose.simoes@eclo.solutions>

cw2 approved these changes Mar 6, 2017

View reviewed changes

cw2 merged commit 4884fe5 into nanoframework:master Mar 6, 2017

josesimoes deleted the add-HAL_Time_TicksToTime branch March 7, 2017 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement HAL_Time_TicksToTimeMilliSec #145

Implement HAL_Time_TicksToTimeMilliSec #145

josesimoes commented Mar 2, 2017 •

edited

Loading

nfbot commented Mar 2, 2017

cw2 commented Mar 2, 2017

josesimoes commented Mar 2, 2017

cw2 commented Mar 2, 2017

cw2 commented Mar 2, 2017 •

edited

Loading

josesimoes commented Mar 2, 2017

josesimoes commented Mar 2, 2017

cw2 commented Mar 2, 2017

cw2 Mar 2, 2017

josesimoes Mar 2, 2017

cw2 Mar 2, 2017

cw2 Mar 2, 2017

josesimoes Mar 2, 2017

cw2 Mar 2, 2017

josesimoes Mar 2, 2017 •

edited

Loading

cw2 Mar 2, 2017

josesimoes Mar 2, 2017

cw2 Mar 2, 2017

josesimoes commented Mar 2, 2017

cw2 commented Mar 2, 2017

cw2 commented Mar 2, 2017

josesimoes commented Mar 2, 2017 •

edited

Loading

cw2 commented Mar 2, 2017

cw2 Mar 2, 2017

cw2 Mar 2, 2017

josesimoes Mar 2, 2017

cw2 Mar 2, 2017

josesimoes Mar 2, 2017

josesimoes Mar 2, 2017

cw2 Mar 2, 2017

josesimoes Mar 2, 2017 •

edited

Loading

Implement HAL_Time_TicksToTimeMilliSec #145

Implement HAL_Time_TicksToTimeMilliSec #145

Conversation

josesimoes commented Mar 2, 2017 • edited Loading

nfbot commented Mar 2, 2017

cw2 commented Mar 2, 2017

josesimoes commented Mar 2, 2017

cw2 commented Mar 2, 2017

cw2 commented Mar 2, 2017 • edited Loading

josesimoes commented Mar 2, 2017

josesimoes commented Mar 2, 2017

cw2 commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josesimoes Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josesimoes commented Mar 2, 2017

cw2 commented Mar 2, 2017

cw2 commented Mar 2, 2017

josesimoes commented Mar 2, 2017 • edited Loading

cw2 commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josesimoes Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

josesimoes commented Mar 2, 2017 •

edited

Loading

cw2 commented Mar 2, 2017 •

edited

Loading

josesimoes Mar 2, 2017 •

edited

Loading

josesimoes commented Mar 2, 2017 •

edited

Loading

josesimoes Mar 2, 2017 •

edited

Loading