feat: use c++ - #7
Conversation
|
Don't merge yet, this needs a lot more testing. |
Discussed internally, this is a rather large change.
ncorrea210
left a comment
ncorrea210
left a comment
There was a problem hiding this comment.
Haven't looked through it all 100% of the way, these are some initial thoughts though. Main thing is that private functions (private to the .cpp file) should probably be static, and I am not sure about this using Span = ... stuff.
| virtual ~Flash() = default; | ||
| virtual bool init() = 0; | ||
| uint32_t mount(); | ||
| uint32_t unmount(); | ||
| uint32_t bootcount(bool update); | ||
| uint32_t open(lfs_file_t* file, const char* filename); | ||
| uint32_t close(lfs_file_t* file); | ||
| bool append(lfs_file_t* file, const uint8_t* bytes, size_t size); | ||
| bool ready() const { return is_ready; }; |
There was a problem hiding this comment.
Would be good to add a quick brief as to what these functions do.
| /// Span --- | ||
| /// A quick primer on span, it just a regular C buffer but it also | ||
| /// includes the size, really convenient, we will be using this a lot | ||
| using Span = std::span<uint8_t>; | ||
| using ConstSpan = std::span<const uint8_t>; |
There was a problem hiding this comment.
Why not directly use std::span?
There was a problem hiding this comment.
This enforces byte buffers, also needs a bit less typing.
There was a problem hiding this comment.
I guess. Not sure if I am a big fan though. @dmanslick thoughts?
There was a problem hiding this comment.
I am fine with removing it, though that does mean we have to do do std::span<uint8_t>(...) instead.
|
overall though it looks nice, am enjoying the cpp |
|
I don't think I understand the purpose of this "Platform" namespace. If it is going to be used everywhere, what is the benefit of the namespace besides knowing that these classes are from the common drivers repo? And if that's its purpose, I think calling the namespace "Common" is better suited. In any case, doesn't it make more sense for sensors to be in their own "Sensors" namespace, protocols to be in their own "Protocols" namespace, etc? |
|
It is a namespace collision thing, but right now it is more of a convention. Libraries generally add a namespace to all their exports so it is something like library_name::module::item. I am fine with removing it, or calling it something else. Technically your sensor example would be Platform::Sensors::BMP581. I just slapped everything under a Platform cause I don't see too much of a reason for sub namespaces yet. |
3 participants
Discussed internally, this is a rather large change. Really sorry about that. Couple of changes
There are some interesting stuff for the
Protocolabstract class, feel free to criticize and discuss. One cool benefit of this rewrite is that we no longer have #ifdefs anywhere outside ofhal.h.