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

EOL refactor prototype #183

Draft
wants to merge 14 commits into
base: ayab-esp32
Choose a base branch
from
Draft

EOL refactor prototype #183

wants to merge 14 commits into from

Conversation

VIPQualityPost
Copy link

@VIPQualityPost VIPQualityPost commented Feb 6, 2024

What do you think?
The main idea is to remove the filtering from the encoder code, and instead only work with a new enum HallState which indicates if the hall sensor is inactive, north, or south. This successfully builds for both Uno and ESP32 environments. I still need to check if I associated the right transitions to the right carriages.

I have also snuck in a small change here to the ayab-esp32 board definition of PIO, changing partition to "default.csv" as the reference board I took it from was outdated.

Copy link

github-actions bot commented Feb 6, 2024

Test Results

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 18fedbe. ± Comparison against base commit aa76597.

@@ -75,7 +75,7 @@ enum class AYAB_API : unsigned char {
using AYAB_API_t = enum AYAB_API;

// API constants
constexpr uint8_t INDSTATE_LEN = 10U;
constexpr uint8_t INDSTATE_LEN = 12U;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please update the API description in ayab-manual accordingly
  • Is there a change on the desktop side required as well to be compatible to this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are also required in the documentation of the API https://github.com/AllYarnsAreBeautiful/ayab-manual/blob/1.0.0-dev/docs/API.md

@@ -51,15 +51,72 @@ void Encoders::encA_interrupt() {
* \brief Read hall sensor on left and right.
* \param pSensor Which sensor to read (left or right).
*/
uint16_t Encoders::getHallValue(Direction_t pSensor) {
HallState_t Encoders::getHallValue(Direction_t pSensor) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a HallState is return from now on, we could rename the method to getHallState instead of getHallValue.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I think that makes sense.

@VIPQualityPost
Copy link
Author

Is @jpcornil-git going to create a PR for his new async encoder code?
His is much better than mine, and achieves the same purpose, so we should think about that too. If not, I'm happy to also modify API docs and desktop app.

@t0mpr1c3
Copy link
Contributor

t0mpr1c3 commented Mar 1, 2024

Is @jpcornil-git going to create a PR for his new async encoder code? His is much better than mine, and achieves the same purpose, so we should think about that too. If not, I'm happy to also modify API docs and desktop app.

Consensus was no in the meeting IIRC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants