Fix ignored qualifiers

[BsAtHome]

Running the compile with -Wignored-qualifiers results in more warnings that indicate wrong types being used. This PR fixes two cases:

• ignored volatile qualifier on cast
• ignored const or volatile return qualifier

Returning const or volatile values is inappropriate, just like casting a value to volatile is inappropriate. The hal_X_t types are replaced with the underlying type and the const on return values is removed.

BTW, yes, I'm trying to eliminate all warnings from the compile.

[andypugh]

BTW, yes, I'm trying to eliminate all warnings from the compile.

I think I achieved that once (but at a lower level of compiler pedantry). I appreciate the effort.

[andypugh]

You have been in there a lot recently. What do you think would be the result of replacing all hal/rtapi_u32/u64 with hal/rtapi_unsigned and the same for signed, where hal_unsigned == u64.
Some HAL files using the dev version might have to change, but that could be handled by update_ini. Some components such as stepgen will need their internal accumulators looking at (in many cases these are already 64 bits, but they all should be to eliminate the risk of wrapping)

[BsAtHome]

BTW, yes, I'm trying to eliminate all warnings from the compile.

I think I achieved that once (but at a lower level of compiler pedantry). I appreciate the effort.

Well, pedantry is necessary for optimal performance ;-)

More seriously, the codebase should compile warning free with -Wextra and then we also should add -Werror by default. There is no excuse for warnings. Warnings are (potential) bugs and generally bad coding practice. There is no need for any warning to be present, at all.

[BsAtHome]

You have been in there a lot recently. What do you think would be the result of replacing all hal/rtapi_u32/u64 with hal/rtapi_unsigned and the same for signed, where hal_unsigned == u64.

I can see the argument, but I don't think it is a good idea. There is a reason for distinct types at the low level of interfacing done by parts of the code. This is, after all, real-time code, where speed should be considered. Therefore, it matters what type you use and how it is aligned.

The real problem is that a lot of code has been written without proper thought why a specific type is used. That is not only true for 32/64 sizes, but also indiscriminate use of signed/unsigned. Also, because the hal_X_t types are special, it matters how you use them (that goes both for atomicity and volatility). They are not normal variables and the compiler is in large part not allowed to optimize the code when you use them.

Just replacing 32 with 64 bit types does not help. You actually need to understand the problem you are trying to solve and pick the right types for the problem at hand.

Some HAL files using the dev version might have to change, but that could be handled by update_ini. Some components such as stepgen will need their internal accumulators looking at (in many cases these are already 64 bits, but they all should be to eliminate the risk of wrapping)

I think a complete review is in order with a clear strategy how this stuff should be coded. But I think we need to create an issue for hal and RT use and discuss the details there.

[andypugh]

My feeling is that on 64-bit CPUs there is probably no difference in speed between 32-bit and 64-bit operations.

The background to this is that initially LinuxCNC used 32-bit floats and 32-bit ints. At some point all the floats were switched to double (and this was before 64-bit CPUs were ubiquitous). Then much later the 64-bit ints were introduced in addition to the 32-bit ones. The 2.9 branch does not have 64-bit HAL pins.
My feeling is that we should not have introduced 64 bits alongside 32, we should have made a clean switch to 64-bit ints in master. And it's not (yet) too late to do that.
This should be pretty much invisible to the users. Some kludges may be needed, LUT5 would become LUT6 for example, but that can be handled in the update_ini script.

[andypugh]

For further discussion see #3286