ads131m0x: Optimized timer scheduling to attain higher sampling rates

[dmbutyugin]

The current scheduling appears to be suboptimal in that it may miss or report FIFO overruns when the MCU resources still allow to attain the corresponding sampling rate. For example, with Mellow ALPS board

[load_cell_probe]
sensor_type: ads131m02
cs_pin: fly_alps:PA15
spi_bus: spi1_PB4_PB5_PB3
spi_speed: 4000000
data_ready_pin: fly_alps:PB6
gain: 128
pwm_clock: static_pwm_clock fly_alps_clk
sample_rate: 8000
adc_channel: 0
counts_per_gram: 67.726962
reference_tare_counts: 797253
sensor_orientation: inverted
speed: 5
lift_speed: 15
trigger_force: 100.0
tare_time: 0.1
z_offset: 0.0
samples: 5
sample_retract_dist: 5.0
samples_result: average
samples_tolerance_retries: 10
samples_tolerance: 0.04

[static_pwm_clock fly_alps_clk]
pin: fly_alps:PA3
frequency: 8000000

the current code reports FIFO overflows, while the MCU waketime is still below 60% (see the charts below for waketime):

Sensor reported errors: 0 errors, 15 overflows
Samples Collected: 77916
Measured samples per second: 7812.9, configured: 7812.5
Good samples: 77916, Saturated samples: 0, Unique values: 14549
Sample range: [9.61% to 10.02%]
Sample range / sensor capacity: 0.20522%

The updated code can maintain that ~8 kSPS without issues. The new code follows the conventions for scheduling used by accelerometers, e.g. ADXL345 or MPU9250, but it retains the same wakeup times from the other loadcells to ensure quick reading of the new data once it becomes available.

Here's the comparison of the MCU wake times and load for the old and the new code:

sample_rate	base	test
500
1000
2000
4000
8000

So, as one can see, the test code results in a slightly higher MCU waketime, but the increase is not very large. And while one can argue that being able to get 8 kSPS is not useful, the changes also mean that less capable MCUs will be able to attain higher sampling rates quite below 8 kSPS.

[KevinOConnor]

Interesting. I'm a little surprised though, because I'd expect the current interface to generally be better at handling high update rates.

I'm guessing that the 2-entry buffer in the adc chip is confusing the scheduling - as it'll sleep the timer for 8/10ths of an update if it detects activity. That amount of time may be too much if there is a buffered entry.

If so, though, I think something like the following would be simpler and more scalable:

--- a/src/sensor_ads131m0x.c
+++ b/src/sensor_ads131m0x.c
@@ -172,6 +172,11 @@ ads131m0x_read_adc(struct ads131m0x_adc *ads131m0x, uint8_t oid)
     trigger_analog_update(ads131m0x->ta, raw);
     buffer_append_int32(&ads131m0x->sb, raw);
     ads131m0x_flush_buffer(ads131m0x, oid);
+
+    if (ads131m0x_is_data_ready(ads131m0x)) {
+        sched_wake_task(&wake_ads131m0x);
+        ads131m0x->pending_flag = 1;
+    }
 }
 
 // Create an ads131m0x sensor

That is, I think one could merge the benefits of high frequency data ready checks with an additional buffer flushing check.

-Kevin

[KevinOConnor]

Just FYI, modern MCUs are really slow when doing math on uint8_t and uint16_t. The ARM mcus don't have 8bit or 16bit math operations (like add, subtract, shift, multiply). So, any math done on a uint16_t or uint8_t the resulting assembler has to scale the value to a uint32_t, perform the actual math, and then truncate the results back to 8bit (or 16bit).

So, in a nutshell, you almost assuredly want to make all these values uint32_t and change the last line to be return crc & 0xffff. Alternatively, if you care about performance on AVR (it's hard to imagine someone hooking one of these sensors up to an old 8-bit AVR cpu, but whatever) then you'll likely want to use uint_fast16_t and still change the last line to return crc & 0xffff.

Cheers,
-Kevin

[dmbutyugin]

If so, though, I think something like the following would be simpler and more scalable:

--- a/src/sensor_ads131m0x.c
+++ b/src/sensor_ads131m0x.c
@@ -172,6 +172,11 @@ ads131m0x_read_adc(struct ads131m0x_adc *ads131m0x, uint8_t oid)
     trigger_analog_update(ads131m0x->ta, raw);
     buffer_append_int32(&ads131m0x->sb, raw);
     ads131m0x_flush_buffer(ads131m0x, oid);
+
+    if (ads131m0x_is_data_ready(ads131m0x)) {
+        sched_wake_task(&wake_ads131m0x);
+        ads131m0x->pending_flag = 1;
+    }
 }
 
 // Create an ads131m0x sensor

Alas, this change alone seems to be insufficient, and reports occasional buffer overflows on my setup when I request 8 kSPS, as I described above. I suppose this is because of the lines

ads131m0x_event(struct timer *timer)
{
    .............
    if (ads131m0x->pending_flag) {
        ads131m0x->sb.possible_overflows++;
        rest_ticks *= 4;
    } else if (ads131m0x_is_data_ready(ads131m0x)) {
    ...........

Basically, this part of event handler will report overflow errors even though a pending flag may be set by an attempt to reschedule a task by ads131m0x_read_adc - so this may not be an overflow yet. In general, I find it a bit difficult to synchronize event handler with the task processing in this scenario (but it may be just me, so let me know if you have ideas how to easily address that). But in contrast, the code in this PR maintains an invariant - either a timer event is scheduled for a sensor, but a corresponding task is inactive, or vice versa, but never simultaneously (so, they schedule/wake-up each other).

That is, I think one could merge the benefits of high frequency data ready checks with an additional buffer flushing check.

I'm sorry, I did not get what you meant by that. The current mainline code does fast high frequency data ready checks indeed (when data_ready signal is not raised by the sensor), and the same is true in my proposal.

[KevinOConnor]

Alas, this change alone seems to be insufficient, and reports occasional buffer overflows on my setup when I request 8 kSPS, as I described above.

Is the issue that it is reporting unnecessary warnings while the data is actually there, or is the issue that it can't keep up with the desired data rate? That is, if you dump 10 seconds of data, do you get a bunch of warnings while having the full 80000 samples - or are you getting 79000 samples which indicates a lot of losses?

If the issue is just unnecessary warnings, you might want to loosen the warning - for example, something like:

--- a/src/sensor_ads131m0x.c
+++ b/src/sensor_ads131m0x.c
@@ -21,7 +21,7 @@ struct ads131m0x_adc {
     uint32_t rest_ticks;
     struct gpio_in data_ready;
     struct spidev_s *spi;
-    uint8_t pending_flag, data_count, was_reset;
+    uint8_t pending_flag, pending_count, was_reset;
     uint8_t channel, num_channels;
     struct sensor_bulk sb;
     struct trigger_analog *ta;
@@ -100,12 +100,20 @@ ads131m0x_event(struct timer *timer)
             timer, struct ads131m0x_adc, timer);
     uint32_t rest_ticks = ads131m0x->rest_ticks;
     if (ads131m0x->pending_flag) {
-        ads131m0x->sb.possible_overflows++;
+        ads131m0x->pending_count++;
+        if (ads131m0x->pending_count >= 3) {
+            // Sensor has a one sample buffer, so only warn if consecutive delay
+            ads131m0x->sb.possible_overflows++;
+            ads131m0x->pending_count = 0;
+        }
         rest_ticks *= 4;
-    } else if (ads131m0x_is_data_ready(ads131m0x)) {
-        ads131m0x->pending_flag = 1;
-        sched_wake_task(&wake_ads131m0x);
-        rest_ticks *= 8;
+    } else {
+        if (ads131m0x_is_data_ready(ads131m0x)) {
+            ads131m0x->pending_flag = 1;
+            sched_wake_task(&wake_ads131m0x);
+            rest_ticks *= 8;
+        }
+        ads131m0x->pending_count = 0;
     }
     ads131m0x->timer.waketime += rest_ticks;
     return SF_RESCHEDULE;

So, basically only report a possible_overflow if the timer detects 1.6 update times without fifo clear (0.8+0.4+0.4).

I'm sorry, I did not get what you meant by that. The current mainline code does fast high frequency data ready checks indeed (when data_ready signal is not raised by the sensor), and the same is true in my proposal.

I think the querying scheme used in adxl345 and similar works well for chips with pretty large fifos - it enables the mcu code to sleep for prolonged periods with little chance of an overflow. However, if this sensor only has a one sample buffer then I'm not sure it's a good fit.

I have some (relatively minor) concerns:

• It's more mcu overhead to frequently schedule and unschedule events than to reschedule an existing timer.
• It isn't immediately obvious to me if this code could avoid reporting warnings while actually losing data. For example: the task is woken ; due to delays ten samples are lost; task wakes up and reads two samples emptying the buffer ; no warning provided.
• I don't fully understand the ads131m0x->timer.waketime manipulation and I wonder if there is a race there. For example: task woken with ads131m0x->timer.waketime set to 9999 and timer not rescheduled ; task delayed a bit ; task drains data ; at time 10020 task sets up next wakeup tentatively for 18000, but sees 9999 < 18000 so schedules task at 9999 ; timer too close.

So, I guess, in general having the constant running timer seems like it's less likely to run into issues. (It is a little unnerving to have both the timer and task modifying pending_flag and sched_wake_task() but both of those actions should be atomic.)

Separately, I wouldn't worry too much about false "possible overflow" reports. I'd be much more concerned about lost messages without a warning. In particular, the host timing wont work well if there are lost messages - so a warning is prudent even if no data was actually lost.

Maybe that helps a little,
-Kevin

[github-actions]

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
   If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.