- 
                Notifications
    
You must be signed in to change notification settings  - Fork 720
 
[nrf fromtree] drivers: sensor: qdec: fix QDEC overflow handling #2101
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When bringing the change to sdk-nrf please also fix the nrf_desktop code and look if there are no other dependencies that may get broken.
| key = irq_lock(); | ||
| acc = data->acc; | ||
| data->acc = 0; | ||
| acc = data->fetched_acc; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change will break https://github.com/nrfconnect/sdk-nrf/blob/main/applications/nrf_desktop/src/hw_interface/wheel.c#L119
Fetch is not called there - probably an overlook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out, wheel updated. Could not find any other occurrences beside the one you mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cherry-pick looks good
b2d5a9d    to
    fbc21f1      
    Compare
  
    | 
           @pdunaj I believe nrf_desktop issue has been resolved  | 
    
| 
           @carlescufi Do you know why Commit Tags job fails? I have seen such issue on other PRs and I am unable to resolve it here. I have done no changes to the commits during the cherry-picking. It has occured after a rebase  | 
    
          
 You have cherry-picked commits from your upstream PR instead of those actually merged ones and your commit messages refer to wrong SHAs.  | 
    
          
 Thanks for pointing that out!  | 
    
fbc21f1    to
    8b02983      
    Compare
  
    QDEC sensor driver fails to inform user of the overflow in the ACC register, which makes the most recently fetched data invalid. An error code return has been added to nrfx_qdec_sample_fetch(), that indicates that an overflow has occured, based on oveflow flag. Also, raw_acc field was added in the qdec_nrfx_data structure, to adjust QDEC to sensor API rules - two subsequent sensor_channel_get() calls should will yield the same values. Signed-off-by: Michał Stasiak <[email protected]> (cherry picked from commit 2e6c83d)
…ew api Overflow errorcode is now correctly detected when expected. Subsequent sensor_channel_get() yield the same values, so the check can be no longer ignored. Added a sensor_sample_fetch() where missing for correct sensor_channel_get() calls. Signed-off-by: Michał Stasiak <[email protected]> (cherry picked from commit 9b5260d)
8b02983    to
    6ecd655      
    Compare
  
    
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, ifsensor_sample_fetch()has not been called in the meantime.