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.

Sign up for GitHub

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

Added V-Mapper:Z for flipped orientation panels chains #1014

Merged
merged 14 commits into from
Mar 22, 2020
Merged

Added V-Mapper:Z for flipped orientation panels chains #1014

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1c51f89
Merge pull request #1 from hzeller/master
marcmerlin Mar 18, 2020
5a10b55
Added V-mapper.
marcmerlin Mar 18, 2020
d79903c
Merge branch 'master' of https://github.com/hzeller/rpi-rgb-led-matri…
marcmerlin Mar 21, 2020
d6c8ffb
Merge branch 'hzeller-master'
marcmerlin Mar 21, 2020
51a4b03
Added V-Mapper:Z for flipped orientation panels chains.
marcmerlin Mar 21, 2020
28e12fb
Removed debugging in Vmapper:Z
marcmerlin Mar 21, 2020
b6f6dbd
VmapperZ: fixed debugging removal.
marcmerlin Mar 21, 2020
f4b9eec
Added Vmapper:Z documentation.
marcmerlin Mar 21, 2020
308a2b6
Orient panels the same way in pixel-mapper.cc.
marcmerlin Mar 21, 2020
af718b0
Vmapper: cleanups#1 after review (bool, whitespace...)
marcmerlin Mar 22, 2020
6df947b
Collapse calculations for *matrix_x/y.
marcmerlin Mar 22, 2020
9869469
V-mapper: Collapse 2 levels of ifs into one.
marcmerlin Mar 22, 2020
7ad630d
Added link to internal copy of Vmapper wiring picture.
marcmerlin Mar 22, 2020
60ae39d
V-Mapper:Z: Better picture for wiring illustration
marcmerlin Mar 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 29 additions & 19 deletions examples-api-use/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,16 +291,18 @@ two chains with 8 panels each

(`--led-chain=8 --led-parallel=2 --led-pixel-mapper="U-mapper"`).

#### V-mapper (Vertical arrangement)
#### V-mapper and Vmapper:Z (Vertical arrangement)

By default, when you add panels on a chain, they are added horizontally.
If you have 2 panels of 64x32, you get 128x32. If you wanted 64x64, you can
use a U-mapper, but it would require that some panels are 'upside down', which
might not be desirable.
The V-mapper allows the stacking to be vertical and not horizontal.
If you have 2 panels of 64x32, you get 128x32.
The V-mapper allows the stacking to be vertical and not horizontal and
get the 64x64 you might want.

Unlike the U mapper, all the panels are correct side up, and you need
more cable length as you need to cross back to the start of the next panel.
By default, all the panels are correct side up, and you need more cable length
as you need to cross back to the start of the next panel.
If you wish to use shorter cables, you can add use Vmapper:Z which will give
you serpentine cabling and every other panel will be upside down (see below
for an example).

It is compatible with parallel chains, so you can have multiple stacks
of panels all building a coherent overall display.
Expand All @@ -315,23 +317,31 @@ Here an example with 3 chains of 4 panels (128x64) for a total of about
Viewed looking the LED-side of the panels:

```
[O < I] [O < I] [O < I]
,---^ ,---^ ,---^
[O < I] [O < I] [O < I]
,---^ ,---^ ,---^
[O < I] [O < I] [O < I]
,---^ ,---^ ,---^
[O < I] [O < I] [O < I]
^ ^ ^
#1 #2 #3 Pi connector (three parallel chains of len 4)
Vmapper Vmapper:Z

[O < I] [O < I] [O < I] [I > O] [I > O] [I > O]
,---^ ,---^ ,---^ ^ ^ ^
[O < I] [O < I] [O < I] [O < I] [O < I] [O < I]
,---^ ,---^ ,---^ ^ ^ ^
[O < I] [O < I] [O < I] [I > O] [I > O] [I > O]
,---^ ,---^ ,---^ ^ ^ ^
[O < I] [O < I] [O < I] [O < I] [O < I] [O < I]
^ ^ ^ ^ ^ ^
#1 #2 #3 #1 #2 #3
Pi connector (three parallel chains of len 4)
```

(This is also a good time to notice that such large arrangement is probably an
(This is also a good time to notice that 384x256 with 12 128x64 panels, is probably an
upper limit of what you can reasonably output without having an unusable fresh
rate (Try these options to help: --led-pwm-bits=7 --led-pwm-dither-bits=1 and get about 100Hz)).

This can also be used to improve the refresh rate of a long display even
if it is only one panel high (e.g. for a text running output) by
This shows the wiring of a 3x5 Vmapper:Z array built by Marc MERLIN, using 15x 64x32 panels:
![IMG_20200320_174436](https://user-images.githubusercontent.com/1369412/77215646-80838200-6ad2-11ea-977a-9d903933376c.jpg)
marcmerlin marked this conversation as resolved.
Show resolved Hide resolved
With --led-pwm-bits=7 --led-pwm-dither-bits=1, it gets a better 300Hz refresh
but only offers around 31K pixels instead of 98K pixels in the previous example.

Please note that Vmapper can also be used to improve the refresh rate of a long
display even if it is only one panel high (e.g. for a text running output) by
splitting the load into multiple parallel chains.

```
Expand Down
34 changes: 30 additions & 4 deletions lib/pixel-mapper.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,14 @@ class VerticalMapper : public PixelMapper {
virtual bool SetParameters(int chain, int parallel, const char *param) {
chain_ = chain;
parallel_ = parallel;
// optional argument :Z allow for every other panel to be flipped
// upside down so that cabling can be shorter:
// [ O < I ] without Z [ O < I ]
// ,---^ <---- ^
// [ O < I ] [ I > O ]
// ,---^ with Z ^
// [ O < I ] ---> [ O < I ]
z_ = (param && strcmp(param, "Z") == 0) ? 1 : 0;
return true;
}

Expand All @@ -237,10 +245,10 @@ class VerticalMapper : public PixelMapper {
*visible_width = matrix_width * parallel_ / chain_;
*visible_height = matrix_height * chain_ / parallel_;
#if 0
fprintf(stderr, "%s: C:%d P:%d. Turning W:%d H:%d Physical "
"into W:%d H:%d Virtual\n",
GetName(), chain_, parallel_,
*visible_width, *visible_height, matrix_width, matrix_height);
fprintf(stderr, "%s: C:%d P:%d. Turning W:%d H:%d Physical "
"into W:%d H:%d Virtual\n",
GetName(), chain_, parallel_,
*visible_width, *visible_height, matrix_width, matrix_height);
#endif
return true;
}
Expand All @@ -250,11 +258,29 @@ class VerticalMapper : public PixelMapper {
int *matrix_x, int *matrix_y) const {
int panel_width = matrix_width / chain_;
int panel_height = matrix_height / parallel_;

*matrix_x = (x % panel_width) + int(y/panel_height)* panel_width;
*matrix_y = (y % panel_height) + int(x/panel_width) * panel_height;
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't it be easier and more readable to figure out if we need flipping first, and then do the one or the other ?

It is very hard to follow to first do the assignment, and then additional modifications later.

So start with an expression that figures out if we should do flipping

const bool needs_flipping = z_ && y / panel_height % 2 == 0;  // or something like that.

And then either some

if (needs_flipping) {
   *matrix_ = ...
   //
} else {
  *matrix_x = (x % panel_width) +  needs_flipping ? ... : int(y/panel_height)* panel_width;
  // ...
}

or, maybe even more readable

 *matrix_x =  (x % panel_width) + needs_flipping ? ... : ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, I'm a bit torn. I'll be honest that I had a hard time figuring out what U-mapper was doing, code that was doing smart things with virtually no comments :)
There are 2 issues maybe in conflict:

  1. efficiency
  2. readability / easy to follow the algorithm flow

For #1: I'll wager that the transform table is computed once at startup and that loosing maybe a millisecond (just made that up) to a few double assigns, is not a prime concern.

For #2:
The function does 3 things
a) normal Vmapper
b) Z Vmapper on a panel that isn't flipped
c) Z Vmapper on a panel that is flipped

I wrote the codeflow to make it very clear which one of a b or c is happening.
By the time you make single line more complex assignments that do a b and/or c at the same time, it's pretty darn hard to figure out what's going on later.

I kind of like the code the way it is because it's pretty simple to follow.
The other part that would be confusing is that x_panel_offset_cnt is computed on the transposed value of X, after it's been mapped to the virtual array (i.e. the first assignment already happened). Sure, you can do smart math that would work before the assignment, but it seemed harder to write, and as a result even harder to re-read.

Keep in mind that we're not top SWEs, so keeping it a bit easier to read, is a plus IMO.
What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, efficientcy we don't care, it only happens once.

But readability. Multiple assignments to things require you to keep track of it when reading. You do and undo things, while all we need to choose is if we need flipping or not, and formulate the expression accordingly.

What happens now is

  • do one thing.
  • oh wait, we are actually doing the other thing .. maybe.
  • we actually need to do the other thing and overwrite which we did first

This is really hard to follow.
So this is why I suggest to change this to

  • do we need to do the other thing ?
  • Yep: do other thing
  • No: do one thing.

And once you've done this, you'll notice, that the ?: version of the same is even more readable.

Copy link
Collaborator Author

@marcmerlin marcmerlin Mar 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Mmmh, so please don't take this as me trying to argue with you :)
The way I wrote it/meant it was:
a) transform from Hmapping to Vmapping
b) if V:Z, see if the resulting panel requires inverting
c) if so, deconstruct the X coordinate to figure out if it ends up on an odd or even panel. If it's an even panel, invert X and Y

a) needs to be done regardless, it's not a) or c), nor does c) undo a)
Combining a and c in one step can be done, but it definitely felt harder to write, and likely not understandable anymore when read.
That said, keep in mind that you're talking to a programmer who isn't as smart as you are (actually I probably barely meet the definition of programmer, and it sure isn't my job description unless you compare with me with java factory factory folks :) ).

If you do see an obvious way to rewrite this in a short and yet still readable way, would you be ok merging this and modifying it the way you had in mind?
(I'll be happy to test your patch before you submit)

Copy link
Owner

Choose a reason for hiding this comment

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

(and yes, calculating from the already transposed value is particularly mind-bending)

Copy link
Owner

Choose a reason for hiding this comment

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

I disagree that a) needs to be done regardless. We either do a) or we do the 'flipped a'. Right now you have to take the one a) and have to do gymnastics to then flip it.

I don't try to make it more complicated or 'smart', I actually suggest this to make it more simple that even I would understand it (right now, it twists my brain).

We can merge it and I can have a look later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"and yes, calculating from the already transposed value is particularly mind-bending" => you'll hate me, I found it easier to vizualize after the transposition than before :)
"We can merge it and I can have a look later." =>
that would be great. I am curious to see how it can be best written.


if (z_) {
marcmerlin marked this conversation as resolved.
Show resolved Hide resolved
int x_panel_offset_cnt = *matrix_x / panel_width;
marcmerlin marked this conversation as resolved.
Show resolved Hide resolved
int x_panel_offset = x_panel_offset_cnt * panel_width;
int y_panel_offset_cnt = *matrix_y / panel_height;
int y_panel_offset = y_panel_offset_cnt * panel_height;

if (x_panel_offset_cnt % 2) {

marcmerlin marked this conversation as resolved.
Show resolved Hide resolved
*matrix_x = panel_width - (*matrix_x - x_panel_offset) - 1;
marcmerlin marked this conversation as resolved.
Show resolved Hide resolved
*matrix_y = panel_height - (*matrix_y - y_panel_offset) - 1;

*matrix_x += x_panel_offset;
*matrix_y += y_panel_offset;
}
}
}

private:
int z_;
marcmerlin marked this conversation as resolved.
Show resolved Hide resolved
int chain_;
int parallel_;
};
Expand Down