ppisa commented on code in PR #12081:
URL: https://github.com/apache/nuttx/pull/12081#discussion_r1564981359
##########
arch/arm/src/samv7/sam_qencoder.c:
##########
@@ -270,11 +316,139 @@ static int sam_reset(struct qe_lowerhalf_s *lower)
static int sam_ioctl(struct qe_lowerhalf_s *lower, int cmd,
unsigned long arg)
{
- /* No ioctl commands supported */
-
+#ifdef CONFIG_SAMV7_QENCODER_ENABLE_GETINDEX
+ switch (cmd)
+ {
+ case QEIOC_GETINDEX:
+ {
+ /* Call the qeindex function */
+
+ sam_qeindex(lower, (struct qe_index_s *) arg);
+ return OK;
+ }
+
+ default:
+ {
+ return -ENOTTY;
+ }
+ }
+#else
return -ENOTTY;
+#endif
}
+#ifdef CONFIG_SAMV7_QENCODER_ENABLE_GETINDEX
+/****************************************************************************
+ * Name: sam_qe_pos_16to32b
+ *
+ * Description:
+ * An inline function performing the extension of current position.
+ * Last reading is saved to priv->last_pos.
+ *
+ ****************************************************************************/
+
+static inline int32_t sam_qe_pos_16to32b(struct qe_lowerhalf_s *lower,
+ uint32_t current_pos)
+{
+ struct sam_lowerhalf_s *priv = (struct sam_lowerhalf_s *) lower;
+
+ uint32_t new_pos = *(volatile uint32_t *) &priv->last_pos;
+ new_pos += (int16_t) (current_pos - new_pos);
+ *(volatile uint32_t *) &priv->last_pos = new_pos;
+
+ return (int32_t) new_pos;
+}
+#endif
+
+#ifdef CONFIG_SAMV7_QENCODER_ENABLE_GETINDEX
+/****************************************************************************
+ * Name: sam_qe_indx_pos_16to32b
+ *
+ * Description:
+ * An inline function performing the extension of the last index position.
+ * Last reading is saved to priv->last_index.
+ *
+ ****************************************************************************/
+
+static inline int32_t sam_qe_indx_pos_16to32b(struct qe_lowerhalf_s *lower,
+ uint32_t current_indx_pos)
+{
+ struct sam_lowerhalf_s *priv = (struct sam_lowerhalf_s *) lower;
+
+ uint32_t new_index = *(volatile uint32_t *) &priv->last_pos;
Review Comment:
The cast can be even to uint16_t but it has to be uintX_t, because modulo
arithmetic is only defined for unsigned types in C language. At the moment,
where two signed types are subtracted then if there has been overflow or result
does not fit into target type then the behavior is undefined. On the other hand
subtraction of unsigned types has precisely defined behavior. In the fact you
get internally signed type which has sign as an additional bit. When size is
limited then it again works with larger unsigned type to have defined behavior
when overflow at or over type module (2 power to n-bits) appears or if there is
underflow under zero.
If the type is not unsigned then following operation after overflow do not
have defined behavior. So that is why the sequence is
```
uint32_t new_index = *(volatile uint32_t *) &priv->last_pos;
new_index += (int16_t) (current_indx_pos - new_index);
*(volatile uint32_t *) &priv->last_index = new_index;
return (int32_t) new_index;
```
When we consider that even for some long distance car track or some pump
control for speed etc. even extended 32-bit value can overflow then even
operations in modulo should be used for controllers in user-space. So it would
be better to return both, IRC value and index as 32-bit unsigned. But because
the NuttX standard is given as signed and for some mechanic with motion within
limited range, it is more natural to use +/- range around zero, we keep signed
output. That is why last_pos is kept as uint32_t even that values visible to
user-space are signed. Copy with volatile then ensures operation on the copy
taken by single read which ensures correct operation even when code is accessed
from multiple threads even on multiple CPUs in parallel.
As for the choice between unit16_t and uint32_t for values which are limited
to int16_t after subtraction, on ARM there are no native operations for 16 or
8-bit wide types, so range limitations has to be introduced by additional
machine operations during each arithmetic operation. Return values and
arguments to the functions are passed in full 32-bit register in the most
cases. So again limitation to 16-bits results in abundant instructions. They
can be eliminated when static inline is used because GCC can find that these
problematic bits do not contribute to the results when intermediate value is
limited to int16_t. Even if we consider x86 in 32-bit and 64-bit modes, then
8-bit and 32-bit operations are natural, 16-bit ones require additional prefix
and cause problematic third (or with CC fourth) input dependency to
instructions. x86 64-operations require REX prefix but there is optimization
for them and 32-bit ones zeros top part so no additional dependency is
introduced. So in
general 16-bit integers are generally problematic for most of today processors
and should be used only to store results into memory if data are large and
limited range is enough. FP16 and Co. and 16-bit vector elements are different
story on GPUs and multimedia extensions...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
