microsoft / pxt-brainpad

Microsoft MakeCode editor for the GHI Brainpad
https://makecode.brainpad.com/
Other
3 stars 11 forks source link

Display pixel order incorrect. #89

Closed greg-norris closed 6 years ago

greg-norris commented 6 years ago

Display is now working but it is displaying the incorrect pixel order as it was originally in the past versions.

p1590549 This example draws two pixels, but the second pixel is set to the wrong location. https://makecode.com/_KjrYFzgdsgXk

p1590548 An example using the butterfly predefined image. https://makecode.com/_VkmECyD246mm

pelikhan commented 6 years ago

The format changed. We have changed the format to match the buffer you expect in your screen. You should be able to pass the buffer we send you as is. Where is the C++ code? Which version of your codal are you using?

greg-norris commented 6 years ago

We are using the correct version on our end. Which sends the buffer as is, we do no processing on the pixels.

This is how we draw point in our test. https://github.com/ghi-electronics/codal-brainpad/blob/master/source/BrainPadDisplay.cpp#L58

greg-norris commented 6 years ago

codal version used in pxt-brainpad.

https://github.com/Microsoft/pxt-brainpad/blob/master/pxtarget.json#L214

pelikhan commented 6 years ago

@mmoskal

mmoskal commented 6 years ago

I was assuming the bit order on your screen was simply column major and little endian. It seems it is somewhat more twisted than that - each byte represents a vertical column of 8 pixels, but these in turn are arranged row-major.

The bad news is that a conversion function will be needed. The good news is that it should be faster since you can move entire bytes.

I'm sorry for the confusion...

You screen needs bytes like this:

1   2   3   ... 127
128 129 130 ... 255
...

and we have bytes like this:

1 8 ... 1016
2 9 ... 1017
3 10 ...

Each byte is 8 vertical pixels. In our layout consecutive (in y direction) pixels are in consecutive bytes, but in the hardware layout they are not.

gus-ghielec commented 6 years ago

It is not just this display. Every display I have came across in the last 20 years uses the same format. This will cause added work for every single make code adopter. Same argument as last time.

Are we going to add conversion a second time and then you guys change again so we have to fix a third time? Just making sure you guys have made final decision this time please.

If it is up to me, make code must match what displays expect, which is an industry standard.

Please take your time thinking about this and give us a final decision. We really need to finalize the BrainPad so we can focus on the curriculum.

mmoskal commented 6 years ago

Sorry about this. The color displays use a regular column-major format, which is what we use (they provide a way of switching this but this is the way they scan, so it's better to stick with that to reduce tearing).

gus-ghielec commented 6 years ago

Ok so the way it is done now is going to be the way going forward?

gus-ghielec commented 6 years ago

Also, have you guys tested the display on the arcade console? The 160x128 display?

mmoskal commented 6 years ago

Yes and yes.

Column major or row major is an arbitrary choice, but this thing with non-consecutive pixels stuff would more tricky to handle, and we want the code for color and mono to match.

mmoskal commented 6 years ago

(the color display scan order is why we changed pixel order - otherwise the tearing effects were very visible)

gus-ghielec commented 6 years ago

@mmoskal can you please fix this to your new format? We would need this to test codal on our end. This is the code we used in the original test.

void DrawPointInPXTFormat(int x, int y, bool set = true) {
    if (x >= 0 && x < 128 && y >= 0 && y < 64) {
        int offset = y * 16;
        offset += x / 8;
        int mask = 0x80 >> (x & 7);
        if (set)
            PXTvram[offset] |= mask;
        else
            PXTvram[offset] &= ~mask;
    }
}
greg-norris commented 6 years ago

We have it fixed now, and working locally, we will be updating our codal-brainpad and then bumping the version at pxt-brainpad.

pelikhan commented 6 years ago

Awesome, great to see that you're getting control of the pipeline!