alternatives to `analogRead`

[t0mpr1c3]

I'm not happy with analogRead to get the active Hall sensor value. The problem is not that it takes an excessively long time for the application, but that it happens during the ISR.

Because the ADC takes a relatively long time (~100 us by default), the ISR (which blocks other interrupts, by default) potentially interferes with Serial communication (which requires an interrupt every byte, or ~90 us at 115,200 Baud). This is potentially the reason for laggy serial communication previously identified (https://github.com/AllYarnsAreBeautiful/ayab-desktop/issues/257).

IMO the optimal solution would be to take the ADC out of the ISR and not use analogRead, even at the penalty of a slightly less portable code base.

This library might be worth a look: https://www.arduino.cc/reference/en/libraries/analogreadasync/

Changes would probably need to be tested on every carriage + KH270.

[t0mpr1c3]

If we are changing the way the ADC is done, I guess the next question would be whether to use it in event driven or free running mode. Free running mode seems like the better option to me, because there will always be a recent value for the ISR to use.

[t0mpr1c3]

A basic implementation would look like this on the UNO https://whileinthisloop.blogspot.com/2016/05/c-in-arduino-isr-in-class.html

[jsarrett]

Generally Free-Running mode is a good choice here, the only things to worry about are that if you want >8-bit values you need to disable interrupts (even for volatile memory accesses) when reading the results from outside the ISR. You'll also have to handle the channel selection MUX if you want to read more than one input. You will need to be conscious of the (relatively minor) delay from when the last conversion was for any specific channel.

It also gives you a chance to potentially make carriage detection asynchronous or at least explicit in the global FSM.

[t0mpr1c3]

There are 2 inputs. So in free running mode I suppose you would typically alternate them?

So, in free running mode there would a change in the timing from ~100 us after the start of the ISR, to somewhere between 0 and ~200 us before the ISR.

(It would be nice for the carriage detection to be more explicit but we can deal with that elsewhere.)

[jpcornil-git]

I like also the approach taken by https://www.arduino.cc/reference/en/libraries/analogreadasync/

analogReadAsync is basically the same as analogRead of the std library but without the busy wait herebelow: https://github.com/arduino/ArduinoCore-avr/blob/master/cores/arduino/wiring_analog.c#L81C35-L81C35

Implementation could then be the following:

• ISR samples ENC_PIN_A/ENC_PIN_B/ENC_PIN_C values, triggers a conversion using analogReadAsync(EOL_PIN_X) and set an update flag for the main loop
• encoder.update() in the main loop has a state machine:
  • ENC_IDLE_STATE: wait for the ISR update flag and when set, run a critical section to copy ISR data/reset update flag and move to ENC_WAIT_ADC
  • ENC_WAIT_ADC: wait for analogReadComplete(), sample hallValue then move ENC_PROCESS_INPUTS
  • ENC_PROCESS_INPUTS: update m_position/m_direction/... as of today in the ISR and return to ENC_IDLE_STATE

[t0mpr1c3]

@jpcornil-git I agree with your very nice analysis. The free-running ADC involves more changes than are necessary.

[jpcornil-git]

Compared to async, it doesn't buy you anything but introduces additional complexity (singled ADC -> you have to manage the input multiplexer, i.e. select EOL sensor when moving to the right and EOR sensor when moving to the left) as well as 100us sampling uncertainty.