Skip to content

add hal_extend_int function #3936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rmu75 wants to merge 1 commit into
base: master
Choose a base branch
from
+53 −0
Open

add hal_extend_int function #3936

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/hal/hal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,24 @@ extern int hal_get_signal_value_by_name(
extern int hal_get_param_value_by_name(
const char *name, hal_type_t *type, hal_data_u **data);

/***********************************************************************
* MISC HELPER FUNCTIONS *
************************************************************************/

/** hal_extend_counter() extends a counter value with nbits to 64 bits.

For some hardware counters it may be desireable to extend their
range beyond their native width.

This function maintains a 64bit counter value and deals with wrap
arounds. Increments between updates need to be less than 2**(nbits-1).
Call with current 64bit counter value to be updated as @param old,
new lower-bit counter value read from hardware as @param newlow
and width of counter as @param nbits.
@returns new counter 64bit counter value.
Code by Jeff Epler. */
Comment thread
rmu75 marked this conversation as resolved.
extern rtapi_s64 hal_extend_int(rtapi_s64 old, rtapi_s64 newlow, int nbits);

Comment on lines +711 to +712
Copy link
Copy Markdown
Contributor

@BsAtHome BsAtHome Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prototype is effectively superfluous unless you try to link in a system that does both not inline and not define that static function locally as static. But, since we are using a C compiler that actually does inline, I do not see the point of the prototype.

Or is there something that I missed?

/***********************************************************************
* EXECUTION RELATED FUNCTIONS *
************************************************************************/
Expand Down Expand Up @@ -983,6 +1001,41 @@ extern bool hal_stream_writable(hal_stream_t *stream);
extern void hal_stream_wait_writable(hal_stream_t *stream, sig_atomic_t *stop);
#endif


/***********************************************************************
* MISC HELPER FUNCTIONS *
************************************************************************/

static inline rtapi_s64 hal_extend_counter(rtapi_s64 old, rtapi_s64 newlow, int nbits)
{
/* Extend low-bit-width counter value to 64bit, counting wrap arounds resp.
"rotations" in additional bits.

see https://github.com/LinuxCNC/linuxcnc/pull/3932#issuecomment-4239206615
This code avoids branches and undefined behaviour due to signed overflow
Idea from MicroPython.

To avoid messy code to sign extend a lower-width value to 64bits, shift
the counter value such that the sign bit gets into the MSB.
Calculate the delta from the stored value in the shifted base.
Shifting back the delta assures the sign is extended properly.
Addition as unsigned avoids signed overflow, which would be undefined
behaviour.

Attention has to be paied that the magnitude of increments / decrements
of the counter stay within 1<<(nshift-1) between calls to this function,
else the wrap around will be counted in the wrong direction.
Comment on lines +1025 to +1027
Copy link
Copy Markdown
Contributor

@BsAtHome BsAtHome Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may think that I am pedantic here, but I'm just reading from a blanked mind (like one from 5 years in the future) and try to fit that into why I would need this function.

It still does not explain what the arguments old and newlow actually are in the context of extending a counter. Which of these arguments is the counter?
The names suggest that there is something "old", but it is unclear what is old. There also is something "new" and apparently also "low" at the same time. But I cannot fathom, from the description, how these names fit into the algorithm.

BTW, the name hal_extend_counter() does seem much more appropriate, but there is more going on than just extending the counter. It is calculating a counter increment from the difference of the arguments and adding that difference to one of the arguments. This exploits the behaviour of unsigned verses two's complement calculations. It uses smaller values as source into a larger register set as if the calculation was performed on a "nbits" register in an unsigned operation (exploiting overflow behaviour) casted to a two's complement result. The final calculation is furthermore (ab)using the unsigned/signed duality. How exactly is this an "easy" algorithm to follow?

Copy link
Copy Markdown
Collaborator Author

@rmu75 rmu75 Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment next to prototype. This comment here in the impl explains what the code is doing and is inside. IDE will display the other comment if oyu hover over a function call.

In fact it doesn't matter if you work with signed or unsigned counter values, the important thing is that the difference has the proper sign and is calculated with width nbits. instead of figuring out the high bits, shifting it up gets proper overflow behaviour and the "extra (0) bits" towards LSB don't matter.

Copy link
Copy Markdown
Collaborator Author

@rmu75 rmu75 Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before i force push and trigger yet another senseless CI run what about

    /* Extend low-bit-width counter value to 64bit, counting wrap arounds resp.
       "rotations" in additional bits. 

       see https://github.com/LinuxCNC/linuxcnc/pull/3932#issuecomment-4239206615
       This code avoids branches and undefined behaviour due to signed overflow
       Idea from MicroPython.

       To avoid messy code to sign extend and overflowa lower-width value to
       64bits, this code shifts the counter value to the end of a 64bit int.
       Thus the calculated delta value overflows correctly and gets the correct
       sign.

       Shifting back the delta assures the sign is extended properly.
       Addition as unsigned avoids signed overflow, which would be undefined
       behaviour.

       Attention has to be paid that the magnitude of increments / decrements
       of the counter stay within 1<<(nshift-1) between calls to this function,
       else the wrap around will be counted in the wrong direction.

       Heuristically, if your encoder can do more than half a rotation between
       calls to this function, it is impossible to deduce which direction it
       went.

       Code contributed by Jeff Epler.

       Example with 3bit absolute hardware encoder and 8bit extended counter
       (nshift would be 5):

       counter at 7, transition from 111 (7) to 000 (0) should increment the
       extended counter to 8.

       call hal_extend_counter(7, 0, 3)
       oldlow_shifted = 7<<5 = 224 (1110 0000)
       newlow_shifted = 0<<5 = 0
       delta_shifted = 0 - 224 = -224 (1 0010 0000) = 32 (0010 0000) truncated to 8 bits 
       old + (32 >> 5) = 7 + 1 = 8
    */


Code contributed by Jeff Epler.
*/
int nshift = 64 - nbits;
rtapi_u64 oldlow_shifted = ((rtapi_u64)old << nshift);
rtapi_u64 newlow_shifted = ((rtapi_u64)newlow << nshift);
rtapi_s64 diff_shifted = newlow_shifted - oldlow_shifted;
return (rtapi_u64)old + (diff_shifted >> nshift);
}


RTAPI_END_DECLS

#endif /* HAL_H */
Loading