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

Giving the library the final polish #35

Open
BrainStone opened this issue Jul 22, 2020 · 3 comments
Open

Giving the library the final polish #35

BrainStone opened this issue Jul 22, 2020 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@BrainStone
Copy link
Contributor

I think you might have noticed by now that I really like your library.

And at this point I'd like to give something back to you.
Now I have some decent experience designing libraries and software in general and I have a few suggestions (that I'm all willing to implement myself):

  • Use inheritance for the specializations:
    This is a fun one! Now I like that you separated the package code into its own class. That's good design. However there is one (fairly) big flaw with how you're doing it right now: The classes don't have anything to do with eachother.
    So my suggestion is to make use of inheritance. You have the abstract Packet class (maybe it should be renamed?) that has the pure virtual functions readAvailable (how many bytes are available to read or maybe a plain boolean function is enough), readByte (reads the next available byte), writeBytes (writes the bytes) and printDebug (which prints the debug message (may be just virtual with a default implementation that does nothing)).
    This allows the library to be extended easily to any other hardware platform and the base class can be made without any external dependencies so it can be used for the Linux/Windows variant of the library.
  • Make the CRC library compile time constant.
    That means making sure that the library doesn't need any RAM and that everything is written in the ROM (constexpr magic at play here ;) ). Now with a preprocessor flag I could toggle between a ROM lookup, RAM lookup or live calculation.
    And lastly let's abandon using class instances for this. My suggestion would be to use only static members in a class. If you really want the size and polynomial to be configurable that can be achieved with templates.

Now if you aprove of these suggestions I'll get on to implementing them.

@BrainStone
Copy link
Contributor Author

This is how to implement the lookup table with constexpr: https://stackoverflow.com/a/34465458/1996022
(Mostly for future reference to myself)

@PowerBroker2 PowerBroker2 self-assigned this Jul 22, 2020
@PowerBroker2 PowerBroker2 added the enhancement New feature or request label Jul 22, 2020
@PowerBroker2
Copy link
Owner

PowerBroker2 commented Jul 22, 2020

I remember thinking about doing inheritance before the big update and decided against it, probably because I completely forgot about virtual functions. I think, however, I can implement inheritance for at least the UART portion of the lib - I'll have to do more looking into SPI/I2C to see if it'll work there too (I assume it'll work, but I want to make sure).

As for the constexpr tip, unfortunately the IDE uses C++11. Because of this, the compiler won't allow you to use for-loops with constexpr functions, which is needed to create arrays (see here). If I could, I would reimplement the CRC lib to evaluate at compile time, but with this complication, I'd rather stick to how I have it now (unless you have another idea).

@PowerBroker2
Copy link
Owner

Now if you aprove of these suggestions I'll get on to implementing them.

Absolutely, feel free to fork and PR - I'm interested to see how good this lib can get!

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

No branches or pull requests

2 participants