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.

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

Asynchronous analog reads #161

Draft
wants to merge 29 commits into
base: main
Choose a base branch
from
Draft

Conversation

t0mpr1c3
Copy link
Contributor

@t0mpr1c3 t0mpr1c3 commented Sep 29, 2023

Closes #160, depends on #154. May fix laggy serial communication identified previously (AllYarnsAreBeautiful/ayab-desktop#257).

Removes analogRead from ISR with minimal changes to current code. analogReadAsync() is portable across AVRs.

Wrapper for analogReadAsync() provides an example of how to mock library functions, relevant to #57.

Test coverage 100% excluding the thin wrapper.

@t0mpr1c3 t0mpr1c3 force-pushed the async branch 2 times, most recently from 14f43e3 to 443ac12 Compare September 29, 2023 21:18
@t0mpr1c3 t0mpr1c3 linked an issue Sep 29, 2023 that may be closed by this pull request
@t0mpr1c3 t0mpr1c3 force-pushed the async branch 16 times, most recently from 0ba8859 to f15b454 Compare October 1, 2023 07:45
@t0mpr1c3 t0mpr1c3 changed the title [DRAFT] Asynchronous analog reads Asynchronous analog reads Oct 1, 2023
@t0mpr1c3
Copy link
Contributor Author

t0mpr1c3 commented Oct 2, 2023

I wouldn't use the callback but the polling approach:

* better not to delay reading of digital data (100us timing offset wrt brother timing now)

* better to run the bulk of the logic cycles in the main loop rather than in an ISR context (no urgency, digital and analog data have all been sampled at the right time/ENCA edge)

I agree, the callbacks should only be used if they are fast enough that this shouldn't be a concern. I assume they probably are fast enough but I admit I haven't checked.

@jpcornil-git
Copy link
Contributor

I have a few days off coming up and will try to make time available to refactor the master branch in a week or two (the one used by my daugther) to move everything but digital data sampling and ADC async trigger out of ISR. I'll also add an async LED implementation (cleaner but also more effective to display state, alive blink, communication activity, ...)

@t0mpr1c3
Copy link
Contributor Author

t0mpr1c3 commented Oct 2, 2023

I have a few days off coming up and will try to make time available to refactor the master branch in a week or two (the one used by my daugther) to move everything but digital data sampling and ADC async trigger out of ISR. I'll also add an async LED implementation (cleaner but also more effective to display state, alive blink, communication activity, ...)

I agree we could do more with the LEDs than we do at the moment.

I like the "heartbeat" but you might want to ask around on the Discord to see if people have other ideas.

@jpcornil-git
Copy link
Contributor

@t0mpr1c3

Just checked in an example illustrating (an async LED implementation) usage of gtest not tainting the code/code tree with global objets and based on platformio (that is also taking care of gtest install and execution), see https://github.com/jpcornil-git/ayab-gtest

I used also my own Arduino mock to avoid dependencies to 3rd party/likely unmaintained repo as it is trivial to implement/extend (see lib/hal/hal.cpp).

All fles (code and header) common to operational and test builds are placed in lib/ to follow platformio convention, operational-only files are placed in src/ and include/ while test files are in test/

I plan now to refactor master (the one used by my daugther implementing the carriage drift fix, G carriage detection and located at https://github.com/AmandineCornil/ayab-firmware) to:

  • add support for gtest using above architecture, isolate all arduino calls with a hal interface, ...
  • convert it to async (beeper, led, analogRead, ...)
  • fix the missing critical section and move most of the encoder processing out of ISR, i.e. isr will just sample the 3 digital inputs and trigger an analogRead everything else will move to async/background task.

I prefer to start from that repo/rev because I don't want to be influenced by current gtest architecture/dependencies/..., it is stable and I understand that code 100%.

It will likely take a few full days to code but, with only 1h here and there, a few weeks to effectivly implement that :-)

@t0mpr1c3 t0mpr1c3 marked this pull request as draft October 19, 2023 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

alternatives to analogRead
2 participants