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

map.cpp - Fix regression for "up" direction, as used by clocks #225

Closed
8 of 9 tasks
henrygab opened this issue Dec 13, 2021 · 3 comments
Closed
8 of 9 tasks

map.cpp - Fix regression for "up" direction, as used by clocks #225

henrygab opened this issue Dec 13, 2021 · 3 comments
Labels

Comments

@henrygab
Copy link
Collaborator

henrygab commented Dec 13, 2021

This is to capture the discussion regarding which way is defaulting to "up" on each board. Specifically, as part of PR #213, "up" was defined to use 64u - hour (etc.). See commit 0702b26, file map.cpp.

However, IIRC, the original branches used different values for "up" when defining the clock's location for 3/6/9/12. The issue tracks the review of the following two functions, back to the original (per-product) branches:

  • drawAnalogClock()
  • drawSpiralAnalogClock()

The branches in question are:

  • 1628-rings -- Map.h used 255 - hour
  • kraken64 -- Map.h used 255 - hour
  • parallel -- N/A, map.h did not exist in that branch
  • fibonacci256 -- Map.h used 64 - hour
  • fibonacci512mini -- Map.h used 255 - hour
  • fibonacci128 -- Map.h used 255 - hour
  • fibonacci64 -- Map.h used 255 - hour
  • fibonacci32mini -- Map.h used 255 - hour

As can be see, every branch calculated "up" as 255 - hour, with the exception of Fibonacci256. Only Fibonacci256 calculated it as the currently-checked-in code does (using 64u - hour). As an aside, it would likely be more correct to use 256u - hour ... casting to uint8_t to cause angle to remain in range [0..255] ... for the other products.

This leaves only one question:

  • Decide how to fix this, or have regressions in all boards other than Fibonacci256
@henrygab
Copy link
Collaborator Author

henrygab commented Dec 13, 2021

@jasoncoon -- I can see two simple ways to fix this.

  1. rotate the positions / angles of Fibonacci256, so that up is at zero degrees (256u ... matching all others)

  2. define new symbol WHICH_WAY_IS_UP for configuration, used only by Fibonacci256

I'm leaning towards the first option. It would entail the following:

A. Subtract 64 from the values stored in angles
B. Rotate values stored in both coordsX and coordsY counter-clockwise 90 degrees. Should be as simple as (untested):
* newX[i] := 255 - oldY[i]
* newY[i] := oldX

Which would you prefer?

@henrygab henrygab changed the title map.cpp - Check 90degree rotation of each device map.cpp - Fix regression for "up" direction, as used by clocks Dec 13, 2021
@jasoncoon
Copy link
Owner

I've confirmed the current 64 - hour works for all of the Fibonacci PCBs.

I should enable the clock patterns for 1628 rings, but they probably don't make any sense for kraken and chamaeleon.

@henrygab
Copy link
Collaborator Author

Ok, I'm fine either way... just wanted to ensure it was a conscious decision, and not an accidental regression.

Note that this will change behavior for existing boards that upgrade, by effectively rotating the output 90 degrees. I'll add to the wiki.

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

No branches or pull requests

2 participants