bartoszbielawski / LEDMatrixDriver

A replacement for Arduino's LedControl library
MIT License
74 stars 25 forks source link

Module reversal code revised. #24

Closed quakec closed 5 years ago

quakec commented 5 years ago
bartoszbielawski commented 5 years ago

I have reviewed the code and I'd like to suggest some changes:

  1. Replacing uint8_t for display inversion with an enum (class enum).
  2. _displayRow(uint8_t row): optimize the code a bit, maybe use one of these? https://helloacm.com/how-to-reverse-bits-for-32-bit-unsigned-integer-in-cc/
  3. Can you also base the PR on the newest code?

Thanks, Bartosz

quakec commented 5 years ago

I have reviewed the code and I'd like to suggest some changes:

1. Replacing uint8_t for display inversion with an enum (class enum).

2. _displayRow(uint8_t row): optimize the code a bit, maybe use one of these? https://helloacm.com/how-to-reverse-bits-for-32-bit-unsigned-integer-in-cc/

3. Can you also base the PR on the newest code?

Thanks, Bartosz

Hi Bartosz,

Thanks for getting back to me.

  1. If you prefer enum that is absolutely fine, I favor numbers because I just didn't like having to find them in the driver and type them all out, not very good for less advanced users, and seems like a lot of extra over-head just for a value.

  2. That's funny, I visited at the exact same page when I did a bit of research surrounding the bit reversal, if you have a more optimized method maybe you can run some performance tests and see, the only thing I can see which will speed things up is swapping pairs perhaps.

  3. I will tidy this up later this evening as per your points. Hoping to spend more time on my LEDMatrixPainter :)

If you'd like to get in touch offline you can get me on jameskropkagohlmaƂpagmailkropkacom

James

bartoszbielawski commented 5 years ago

Ad 1. Any modern editor will show you the definition of an enum by hovering over or ctrl-clicking on the type. The gain is strict type control and named values. Ad 2. I don't have time for doing a benchmark but here's assembly output. Also - the complexity is log(n) instead of linear. https://gist.github.com/bartoszbielawski/633af21b841aaf7b3e8565fe47c5554c https://gist.github.com/bartoszbielawski/fa741c724f74a79a9252a3ea096ef720 Ad 3. No hurry :) As you can see I'm not responding immediately as well.

jameskropkagohlmaƂpagmailkropkacom

What if I didn't speak polish? :> :) My e-mail if needed - the login + gmail.