Skip to content
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

[nrf fromtree] drivers: sensor: qdec: fix QDEC overflow handling #2101

Merged
merged 2 commits into from
Oct 21, 2024
Merged

[nrf fromtree] drivers: sensor: qdec: fix QDEC overflow handling #2101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16bfb18
[nrf fromtree] drivers: sensor: qdec: fix QDEC overflow handling
mstasiaknordic Jul 11, 2024
6ecd655
[nrf fromtree] tests: boards: nrf: qdec: modify QDEC tests to match n…
mstasiaknordic Jul 29, 2024
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
23 changes: 21 additions & 2 deletions drivers/sensor/nordic/qdec_nrfx/qdec_nrfx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ LOG_MODULE_REGISTER(qdec_nrfx, CONFIG_SENSOR_LOG_LEVEL);


struct qdec_nrfx_data {
int32_t fetched_acc;
int32_t acc;
bool overflow;
sensor_trigger_handler_t data_ready_handler;
const struct sensor_trigger *data_ready_trigger;
};
Expand All @@ -49,6 +51,8 @@ static void accumulate(struct qdec_nrfx_data *data, int32_t acc)

if (!overflow) {
data->acc += acc;
} else {
data->overflow = true;
}

irq_unlock(key);
Expand All @@ -70,6 +74,18 @@ static int qdec_nrfx_sample_fetch(const struct device *dev,

accumulate(data, acc);

unsigned int key = irq_lock();

data->fetched_acc = data->acc;
data->acc = 0;

irq_unlock(key);

if (data->overflow) {
data->overflow = false;
return -EOVERFLOW;
}

return 0;
}

Expand All @@ -87,8 +103,7 @@ static int qdec_nrfx_channel_get(const struct device *dev,
}

key = irq_lock();
acc = data->acc;
data->acc = 0;
acc = data->fetched_acc;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

irq_unlock(key);

val->val1 = (acc * FULL_ANGLE) / config->steps;
Expand Down Expand Up @@ -148,6 +163,10 @@ static void qdec_nrfx_event_handler(nrfx_qdec_event_t event, void *p_context)
}
break;

case NRF_QDEC_EVENT_ACCOF:
dev_data->overflow = true;
break;

default:
LOG_ERR("unhandled event (0x%x)", event.type);
break;
Expand Down
37 changes: 20 additions & 17 deletions tests/boards/nrf/qdec/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@
}

static void qenc_emulate_verify_reading(int emulator_period_ms, int emulation_duration_ms,
bool forward, bool overflow_possible)
bool forward, bool overflow_expected)
{
int rc;
struct sensor_value val = {0};
Expand All @@ -107,13 +107,17 @@
k_msleep(emulation_duration_ms);

rc = sensor_sample_fetch(qdec_dev);
zassert_true(rc == 0, "Failed to fetch sample (%d)", rc);

if (!overflow_expected) {
zassert_true(rc == 0, "Failed to fetch sample (%d)", rc);
} else {
zassert_true(rc == -EOVERFLOW, "Failed to detect overflow");
}

rc = sensor_channel_get(qdec_dev, SENSOR_CHAN_ROTATION, &val);
zassert_true(rc == 0, "Failed to get sample (%d)", rc);

TC_PRINT("QDEC reading: %d\n", val.val1);
if (!overflow_possible) {
if (!overflow_expected) {
zassert_within(val.val1, expected_reading, delta,
"Expected reading: %d, but got: %d", expected_reading, val.val1);
}
Expand Down Expand Up @@ -197,6 +201,9 @@
rc = k_sem_take(&sem, K_MSEC(200));
zassert_true(rc == 0, "qdec handler should be triggered (%d)", rc);

rc = sensor_sample_fetch(qdec_dev);
zassert_true(rc == 0, "Failed to fetch sample (%d)", rc);

rc = sensor_channel_get(qdec_dev, SENSOR_CHAN_ROTATION, &val);
zassert_true(rc == 0, "Failed to fetch sample (%d)", rc);

Expand Down Expand Up @@ -241,7 +248,6 @@
qenc_emulate_verify_reading(10, 100, true, false);
qenc_emulate_verify_reading(2, 500, true, false);
qenc_emulate_verify_reading(10, 200, false, false);
/* 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);
}
Expand Down Expand Up @@ -313,19 +319,16 @@
/* subsequent calls of sensor_channel_get without calling sensor_sample_fetch
* should yield the same value
*/
/* zassert_true(val_first.val1 == val_second.val1,
* "Expected the same readings: %d vs %d",
* val_first.val1,
* val_second.val1);
*/
TC_PRINT("Expected the same readings: %d vs %d - ignore!\n", val_first.val1,
val_second.val1);
/* zassert_true(val_first.val2 == val_second.val2, "Expected the same readings: %d vs %d",
* val_first.val2, val_second.val2);
*/
TC_PRINT("Expected the same readings: %d vs %d - ignore!\n", val_first.val2,
val_second.val2);
zassert_true(val_first.val1 == val_second.val1,
"Expected the same readings: %d vs %d",
val_first.val1,
val_second.val1);

zassert_true(val_first.val2 == val_second.val2,
"Expected the same readings: %d vs %d",
val_first.val2,
val_second.val2);
}

Check notice on line 331 in tests/boards/nrf/qdec/src/main.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

tests/boards/nrf/qdec/src/main.c:331 
-	zassert_true(val_first.val1 == val_second.val1,
-		     "Expected the same readings: %d vs %d",
-		     val_first.val1,
-		     val_second.val1);
-
-	zassert_true(val_first.val2 == val_second.val2,
-		     "Expected the same readings: %d vs %d",
-		     val_first.val2,
-		     val_second.val2);
+	zassert_true(val_first.val1 == val_second.val1, "Expected the same readings: %d vs %d",
+		     val_first.val1, val_second.val1);
+
+	zassert_true(val_first.val2 == val_second.val2, "Expected the same readings: %d vs %d",
+		     val_first.val2, val_second.val2);

/**
* @brief sensor_channel_get test negative
Expand Down
Loading