drivers: sensor: qdec: fix QDEC overflow handling

[mstasiaknordic]

Fixed overflow handling in nrf QDEC sensor driver. It failed to inform user of an overflow in the ACC register, that caused the fetched sample to be invalid because of discarding data that would cause the overflow.

Now, qdec_nrfx_sample_fetch() returns error code if an overflow occured, based on overflow field in qdec data structure.

Also, driver is now more adjusted to sensor driver API. It is guaranteed that two subsequent calls of sensor_channel_get() function for the same channels will yield the same value, if sensor_sample_fetch() has not been called in the meantime.

[github-actions]

Hello @mstasiaknordic, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[nandojve]

CC: @tswaehn

[tswaehn]

dear @mstasiaknordic

as far as I understand, every qdec has a fixed number of digits, can be register size of ex. 16bit or 32bit. at some point when continue to rotate into same direction, there will be an overflow based on the internal register size. after overflow the counter should automatically continue to count ex. upwards. this may lead a 16bit qdec driver to return 32.767 (0x7FFF) and next value could be -32768 (0x8000). that is from my perspective expected behavior. overflows in number need to be handled by the user of the driver. so the qdec driver only returns the diff qdec counter value.

=> the user will always have the correct counter value when applying the diff from qdec to a self managed huger counter ex. int128_t. here is no actual overflow possible.

in contrast -EOVERFLOW should be returned only, if within two consecutive readings the qdec counter travels a distance of more than ex. 16bit or 32bit (depending on register width). this has nothing to do with crossing 0 or turning positive number into negative number. this is about distance of last vs current reading.

=> this is the actual critical part. if the qdec travels more than the available register width, then the user may lose precision, this is the actual overlow that need to be detected.

so in conclusion:

I think the use case of overflow is miss interpreted in the qdec driver - or my understanding may be different.

[mstasiaknordic]

Thank you for your response @tswaehn

as far as I understand, every qdec has a fixed number of digits, can be register size of ex. 16bit or 32bit. at some point when continue to rotate into same direction, there will be an overflow based on the internal register size. after overflow the counter should automatically continue to count ex. upwards. this may lead a 16bit qdec driver to return 32.767 (0x7FFF) and next value could be -32768 (0x8000). that is from my perspective expected behavior. overflows in number need to be handled by the user of the driver. so the qdec driver only returns the diff qdec counter value.

Problem is that QDEC's internal register does not accept samples that would cause an overflow, so the sign-changing accumulation does not occur, the register remains unchanged while user expects a change of value.

See : https://docs.nordicsemi.com/bundle/ps_nrf5340/page/chapters/qdec/qdec.html#ariaid-title5

My suggested change is that the user would be informed of such overflow happening with an errorcode signalizing that the data fetched by the user using sensor_sample_fetch() is corrupted with the said lack of full data caused by the overflow.

[tswaehn]

dear @mstasiaknordic

from the document it looks like the ACC counter is not turning around when continue to count ex. upwards. instead it is stuck at max or min.

the workaround could be to reset the ACC counter on every readout, this prevents the counter to be stuck at max or min. this reset should be as quick as possible to not lose any counts that fly in while resetting.

additionally there could be a uint16_t or uint32_t software counter that is updated on every reading (sensor_sample_fetch()) with the current ACC value (plus the reset of ACC as above, right after reading).

this would, from my perspective then behave like a "normal" free running qdec and no external api changes required.

additionally one could also report an overflow, if within two consecutive readings the ACC is stuck, because that would imply that a precision failure exists. which means the time of two readings was too long, or the qdec rotated too fast.

what do you think?

[mstasiaknordic]

@tswaehn

the workaround is to reset the ACC counter on every readout, this prevents the counter to be stuck at max or min. this reset should be as quick as possible to not lose any counts that fly in while resetting.

It is already implemented.

additionally there could be a uint16_t or uint32_t software counter that is updated on every reading (sensor_sample_fetch()) with the current ACC value (plus the reset of ACC as above, right after reading).

There is one - qdec_nrfx_data.acc. Is is a signed integer field, since ACC can take negative values.

additionally one could also report an overflow, if within two consecutive readings the ACC is stuck, because that would imply that a precision failure exists. which means the time of two readings was too long, or the qdec rotated too fast.

Agreed, such overflow is now reported by an errorcode returned by sensor_sample_fetch().

[tswaehn]

dear @mstasiaknordic

I need to disagree with you. I cannot find the suggested reset of ACC. additionally the overflow is generated on number overflow and not depending on two consecutive readings. therefore the last reading need to be stored somewhere, to compare with current reading, which I cannot find either.

additionally from my perspective the channel_get() the question is, what values to return. a qdec usually returns increments. in your case you return degrees.

Actually I have the feeling your implementation is a protractor instead of qdec. (protractor can be a specialized qdec, but not vice versa).

usually a qdec can have like 4096 increments per rotation or even much more. this is really application dependant. in many applications you count multiple rotations of motors, forwards and backwards.

[mstasiaknordic]

@tswaehn

I need to disagree with you. I cannot find the suggested reset of ACC.

nrfx_qdec_accumulators_read(&config->qdec, &acc, &accdbl) called in qdec_nrfx_sample_fetch calls nrfy_qdec_task_trigger with TASKS_READCLRACC that reads from the ACC register and clears it.

additionally the overflow is generated on number overflow and not depending on two consecutive readings. therefore the last reading need to be stored somewhere, to compare with current reading, which I cannot find either.

overflow field in qdec_nrfx_data is set whenever an EVENTS_ACCOF occurs which happens between readings. When -EOVERFLOW is returned by qdec_nrfx_sample_fetch, acc field in qdec_nrfx_data is not cleared, thus the last reading is stored. It not updated with the corrupted value of ACC register though, if you meant it.

qdec usually returns increments. in your case you return degrees.

Yes, though it was not designed by me and, I believe, is not the subject of this PR.

[tswaehn]

nrfx_qdec_accumulators_read(&config->qdec, &acc, &accdbl) called in qdec_nrfx_sample_fetch calls nrfy_qdec_task_trigger with TASKS_READCLRACC that reads from the ACC register and clears it.

could you please help me to find the code ?

what is the expected output of sensor value after 2.5x full rotations of the qdec?

[mstasiaknordic]

could you please help me to find the code ?

nrfx_qdec_accumulators_read here with NRF_QDEC_TASK_READCLRACC : https://github.com/zephyrproject-rtos/hal_nordic/blob/bda16da6f92380579b8cf681a5faf61e1fe55220/nrfx/drivers/src/nrfx_qdec.c#L237

said task here : https://github.com/zephyrproject-rtos/hal_nordic/blob/bda16da6f92380579b8cf681a5faf61e1fe55220/nrfx/hal/nrf_qdec.h#L96

what is the expected output of sensor value after 2.5x full rotations of the qdec?

With full you mean 360 degrees or reaching max value of the register?

[tswaehn]

@mstasiaknordic

thank you for point this out. makes sense to me now. also agree to your changes made.
(still have my doubts that this is qdec sensor vs. protractor, also here I agree, not part of this PR)

[mstasiaknordic]

still have my doubts that this is qdec sensor vs. protractor

I have looked at Zephyr QDEC sample, and it expects value returned from sensor to be degrees, thus I assume that driver should be responsible for the convertion from increments to degrees.

[tswaehn]

I have looked at Zephyr QDEC sample, and it expects value returned from sensor to be degrees, thus I assume that driver should be responsible for the convertion from increments to degrees.

wouldnt that loose precision?

[mstasiaknordic]

wouldnt that loose precision?

Yes, if we consider only val1 field of sensor_value, which is the case for the sample. val2 holds the fractional part of value and is updated by the driver.

[tswaehn]

just checked, some qdec drivers return position (increments) and others return degrees.

although your mentioned sample indicates it should be degrees.

		printk("Position = %d degrees\n", val.val1);

• static int qdec_stm32_get() => position
• static int qdec_s32_ch_get() => degrees
• static int qdec_mcux_ch_get() => degrees
• static int qdec_nrfx_channel_get() => degrees
• static int qdec_sam_get() => position

so, you are following the trend

my personal opinion is to have this configurable to get raw units or real-world-units within the sensor configuration, ...

as you mentioned, ... not part of this PR

[anangl]

This is wrong. The overflow is checked against data->acc:

zephyr/drivers/sensor/nordic/qdec_nrfx/qdec_nrfx.c

Lines 49 to 50 in d0d1cfc

	bool overflow = ((acc > 0) && (ACC_MAX - acc < data->acc)) ||
	((acc < 0) && (ACC_MIN - acc > data->acc));

but data->raw_acc is actually modified, so some overflows can be missed and some can be assumed incorrectly.

[anangl]

Why not bool?

[anangl]

This is not the proper way of handling the overflow. The accumulators should still be read in such case.

[mstasiaknordic]

What value should be read in that case? One before applying the corrupted sample?

[anangl]

raw_acc is quite vague. IMO, it would be better to have acc where you accumulate the readouts from the QDEC and e.g. fetched_acc where the accumulated value would be stored after it was fetched.

[anangl]

The accumulated value should be accessed with interrupts locked and the overflow should be reported here instead of before the accumulators reading.

[anangl]

Is the overflow possible or expected? I think it is the latter and the variable name should be corrected, as well as the following comment:

zephyr/tests/boards/nrf/qdec/src/main.c

Lines 251 to 253 in d0d1cfc

	/* may lead to overflows but currently driver does not detects that */
	qenc_emulate_verify_reading(1, 1000, false, true);
	qenc_emulate_verify_reading(1, 1000, true, true);

[mstasiaknordic]

When running the test locally, it always led to overflow, so I agree

[mstasiaknordic]

@anangl could you take a look?