Open
Conversation
… deserialize function
- The union in the ProtocolMessage is now const
- This means the individual fields don't have to be const which makes
the implementation a lot cleaner
- The fields inside the AlarmCommand are const
- Now instances of the AlarmCommand don't need to be const, the fields
are always const, which it should be
- The contents of an AlarmCommand should never change
- Moving all the constructors to just taking std::array<char, size_t> - Removing most of the 'deserialize' methods
- Since the Copy constructor is deleted from the AlarmCommand the Move construcctor must be used - in the ::buildAlarmMessage() method it was not using std::move so the returned AlarmMessage was in a invalid state
…e parts - Since parts of the alarm message, like type designator, are optional, needed a way to indicate they are possibly present, possibly existent
…ment operators - Originally the move assignment operators simply returned `*this` - The operators had to first set the data members inside the operator and THEN return `*this`
- the portable use of `size_t` is actually `std::size_t` - the functions `std::isdigit()` and `std::isxdigit()` belong to the `cctype` library
- including `iostream` greatly increased the size of the binary and ram usage - Going back to `Printable` class but now also mocking it so it can be used in tests
Collaborator
Author
|
I also realize now that this PR is something I would consider "quite large". It is ~1.5k LOC which is difficult on reviewers. What I can say is that a lot of it is just "boiler plate" code of defining classes, header guards, etc. So the actual "meat" of the PR is not as large but it is still getting on the side of too big for a PR. |
Collaborator
Author
|
@RobertLRead I do want to address a point you made once during a discussion that full on unit tests were not entirely necessary. Well, PlatformIO makes it VERY easy to incorporate unit tests into the project and by being able to run the tests on the host system it removes the delay of having to upload a new firmware image to the board and it makes it easier if you wished to debug the code as well. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the initial implementation of the GPAP protocol.
This leverages a number of new techniques and tools that may be foreign to some. Extensive use of "return value optimization" or "copy elision" in the new
GPAPmodule. All of the GPAP related code is stuck inside its own library file structure inside thelib/directory. There is a README in that directory as well which explains some of the foreign concepts used.As well, this pull request introduces the use of unit tests. The unit tests test just the new
GPAPlibrary code. I am free to answer any specific questions about the unit tests but for a good overview and tutorial the official documentation is the best source to learn about PlatformIO's unit testing system. Something that should be explicitly pointed out is the addition of thenativeenvironment in theplatform.inifile. That is because these specific unit tests are compiled for the host system. They can be compiled for theesp32devdevice as well but it was just easier to get it running for the host device. This does require an external C++11 (or newer) compiler on the host system. On *nix based systems this is quite oftengccwhich is easy enough to install. On Windows this leverages MSVC which I have no experience with getting installed.