Module reversal revised + new example

[quakec]

I didn’t put - - In loop because I copy pasted, a cardinal sin yes, but I understood the logic and reviewed it.

Rename vars okay, it’s your driver and it’s imperative the structure and naming conventions are consistent throughout.

Yes, knowledge being my limitation, keeping it working behind the scenes was my goal, exposing the enum to the constructor, it’s so messy and I did say I wasn’t happy with it, I am open to suggestions 😊

“Caching” and doing so at function level is new to me, in what context? The cast is the best I could come up with given my knowledge of what is even possible 😊.

On Tue, 26 Feb 2019 at 23:19, Bartosz Bielawski notifications@github.com wrote:

@bartoszbielawski requested changes on this pull request.

Please check my comments in the code :)

In src/LEDMatrixDriver.cpp https://github.com/bartoszbielawski/LEDMatrixDriver/pull/25#discussion_r260532105 :

@@ -160,13 +160,32 @@ void LEDMatrixDriver::_sendCommand(uint16_t command) SPI.endTransaction(); }

+uint8_t LEDMatrixDriver::_reverseBits(uint8_t n) +{

• uint8_t x;
• for(auto i = 7; n; ) {

Why don't you put --i; in the for loop?

In src/LEDMatrixDriver.hpp https://github.com/bartoszbielawski/LEDMatrixDriver/pull/25#discussion_r260532294 :

@@ -41,9 +41,17 @@ class LEDMatrixDriver

public:

• enum class flipDirection

I'd prefer to have the type name capitalized.

In src/LEDMatrixDriver.hpp https://github.com/bartoszbielawski/LEDMatrixDriver/pull/25#discussion_r260532478 :

const uint8_t N; SPISettings spiSettings;

• bool modRev = false;
• uint8_t moduleFlip;

Now the type internally is the raw one... Ugh, but there's no nice way to handle that I guess.

In src/LEDMatrixDriver.cpp https://github.com/bartoszbielawski/LEDMatrixDriver/pull/25#discussion_r260532713 :

void LEDMatrixDriver::_displayRow(uint8_t row) { SPI.beginTransaction(spiSettings); digitalWrite(ssPin, 0); for (uint16_t d = 0; d < N; d++) {

• uint16_t cmd = ((row + 1) << 8) | frameBuffer[d + row*N];
• uint8_t col = frameBuffer[d + row*N];
• if ((moduleFlip & static_cast(flipDirection::flipColumn)) != 0)

How about caching if it's reversed or not and not checking it every time? You can do it once per function instead of doing it for every element.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bartoszbielawski/LEDMatrixDriver/pull/25#pullrequestreview-208262561, or mute the thread https://github.com/notifications/unsubscribe-auth/ABtJb_NGuJNPl8OMBC595yhIPOokbEHuks5vRcEGgaJpZM4bQ_Ia .

-- Sent from my iPhone 6s A1688 iOS 11.0.3

[bartoszbielawski]

I didn’t put - - In loop because I copy pasted, a cardinal sin yes, but I understood the logic and reviewed it.

It's not about understanding only, it's about making others understand what the code does. Without this one it doesn't look like a standard for loop.

Yes, knowledge being my limitation, keeping it working behind the scenes was my goal, exposing the enum to the constructor, it’s so messy and I did say I wasn’t happy with it, I am open to suggestions 😊 “Caching” and doing so at function level is new to me, in what context? The cast is the best I could come up with given my knowledge of what is even possible 😊.

Okay, maybe I used a wrong word. You can make a const bool hReversed and const bool vReversed members that will be initialized from the enum. Then in the code you can use these constants, no reason to check every time if it's to be inverted or not.

[bartoszbielawski]

Hi, I have just pushed a new commit to a second branch. I decided to go with a mix of your and mine code. I have finally dropper the class enums, I found a nice way to solve it (with std::set) but I don't want to bring STL into the code. So it will stay with uint8_t flags and let's call it a day. I have also added an option to invert whole display horizontally, not only separate segments. Feel free to check the new code. If everything's fine I will make an official release.

[quakec]

Sounds good :)

Have you merged into master yet? I can't see it.

Also do you still have access to my final example? The MarqueeTextClean example is poorly derrived from the MarqueeText example.

Regards,

James Gohl

Are you cyber streetwise?https://www.cyberaware.gov.uk/

On Sat, 16 Mar 2019 at 17:10, Bartosz Bielawski notifications@github.com wrote:

Hi, I have just pushed a new commit to a second branch. I decided to go with a mix of your and mine code. I have finally dropper the class enums, I found a nice way to solve it (with std::set) but I don't want to bring STL into the code. So it will stay with uint8_t flags and let's call it a day. I have also added an option to invert whole display horizontally, not only separate segments. Feel free to check the new code. If everything's fine I will make an official release.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bartoszbielawski/LEDMatrixDriver/pull/25#issuecomment-473566306, or mute the thread https://github.com/notifications/unsubscribe-auth/ABtJbypDAegsTXMdES4lsupt8lSzLjkCks5vXSWfgaJpZM4bQ_Ia .

[quakec]

Please include this and delete MarqueeTextClean.ino :)

Regards,

James Gohl

Are you cyber streetwise?https://www.cyberaware.gov.uk/

On Sat, 16 Mar 2019 at 19:56, James Gohl james.gohl@gmail.com wrote:

Sounds good :)

Have you merged into master yet? I can't see it.

Also do you still have access to my final example? The MarqueeTextClean example is poorly derrived from the MarqueeText example.

Regards,

James Gohl

Are you cyber streetwise?https://www.cyberaware.gov.uk/

On Sat, 16 Mar 2019 at 17:10, Bartosz Bielawski notifications@github.com wrote:

Hi, I have just pushed a new commit to a second branch. I decided to go with a mix of your and mine code. I have finally dropper the class enums, I found a nice way to solve it (with std::set) but I don't want to bring STL into the code. So it will stay with uint8_t flags and let's call it a day. I have also added an option to invert whole display horizontally, not only separate segments. Feel free to check the new code. If everything's fine I will make an official release.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bartoszbielawski/LEDMatrixDriver/pull/25#issuecomment-473566306, or mute the thread https://github.com/notifications/unsubscribe-auth/ABtJbypDAegsTXMdES4lsupt8lSzLjkCks5vXSWfgaJpZM4bQ_Ia .

[bartoszbielawski]

Nope, wanted to have it tested by somebody else than me. It's in a separate branch for now. So it's there but not as the master. So I'm not yet braking all the people's projects.

BB

On Sat, 16 Mar 2019, 20:56 James Gohl, notifications@github.com wrote:

Sounds good :)

Have you merged into master yet? I can't see it.

Also do you still have access to my final example? The MarqueeTextClean example is poorly derrived from the MarqueeText example.

Regards,

James Gohl

Are you cyber streetwise?https://www.cyberaware.gov.uk/

On Sat, 16 Mar 2019 at 17:10, Bartosz Bielawski notifications@github.com wrote:

Hi, I have just pushed a new commit to a second branch. I decided to go with a mix of your and mine code. I have finally dropper the class enums, I found a nice way to solve it (with std::set) but I don't want to bring STL into the code. So it will stay with uint8_t flags and let's call it a day. I have also added an option to invert whole display horizontally, not only separate segments. Feel free to check the new code. If everything's fine I will make an official release.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub < https://github.com/bartoszbielawski/LEDMatrixDriver/pull/25#issuecomment-473566306 , or mute the thread < https://github.com/notifications/unsubscribe-auth/ABtJbypDAegsTXMdES4lsupt8lSzLjkCks5vXSWfgaJpZM4bQ_Ia

.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/bartoszbielawski/LEDMatrixDriver/pull/25#issuecomment-473580196, or mute the thread https://github.com/notifications/unsubscribe-auth/AD2sEAqO10k1Fz3kJmHp-UpfVHml8pDhks5vXUxugaJpZM4bQ_Ia .