SNS - LED Driver - #44
Conversation
TomLonergan03
left a comment
TomLonergan03
left a comment
There was a problem hiding this comment.
looking good, mostly stylistic changes
| std::shared_ptr<io::II2c> i2c, | ||
| const std::uint8_t device_address) | ||
| { | ||
| if (device_address != kDefaultLedDriverAddress) { |
There was a problem hiding this comment.
what's the point of taking an argument for the LED driver if it must match the default?
| logger.log(core::LogLevel::kFatal, "Invalid device address for LED driver"); | ||
| return std::nullopt; | ||
| } | ||
|
|
There was a problem hiding this comment.
avoid using whitespace (see the style guide here). the metaphor is that whitespace in code is similar to whitespace in an essay, it should separate paragraphs but you wouldn't have a line break after every sentence
| { | ||
| } | ||
|
|
||
| std::optional<core::Result> LedDriver::initialise() |
There was a problem hiding this comment.
this doesn't need to be optional, just return core::Result::kFailure/core::Result::kSuccess
There was a problem hiding this comment.
same for all the others that currently return std::optional<core::Result>
| std::optional<core::Result> LedDriver::set_colour(std::uint8_t channel, | ||
| std::uint8_t red, | ||
| std::uint8_t green, | ||
| std::uint8_t blue) |
There was a problem hiding this comment.
all these args should be const
| #include <core/logger.hpp> | ||
| #include <io/i2c.hpp> | ||
|
|
||
| int kDefaultLedDriverAddress = 0x30; |
There was a problem hiding this comment.
this should be static constexpr std::uint8_t
| static std::optional<LedDriver> create(core::ILogger &logger, | ||
| std::shared_ptr<io::II2c> i2c, | ||
| const std::uint8_t device_address | ||
| = kDefaultLedDriverAddress); |
There was a problem hiding this comment.
we don't use default parameters, but here we don't need the param at all so it's a kinda meaningless point to make
| = kDefaultLedDriverAddress); | ||
| ~LedDriver(); | ||
|
|
||
| std::optional<core::Result> initialise(); |
There was a problem hiding this comment.
this should be private i think
| ~LedDriver(); | ||
|
|
||
| std::optional<core::Result> initialise(); | ||
| std::optional<core::Result> set_colour(std::uint8_t channel, |
There was a problem hiding this comment.
functions names should be in lowerCamelCase
| std::uint8_t green, | ||
| std::uint8_t blue); | ||
| std::optional<core::Result> set_intensity(std::uint8_t channel, std::uint8_t intensity); | ||
| std::optional<core::Result> reset(); |
There was a problem hiding this comment.
can you write a docstring for all of these methods, especially reset as it is a little unclear what it does
|
|
||
| LedDriver::LedDriver(core::ILogger &logger, std::shared_ptr<io::II2c> i2c) | ||
| : logger_(logger), | ||
| i2c_(i2c) |
There was a problem hiding this comment.
might need a std::move here?
| core::Result setIntensity(std::uint8_t channel, std::uint8_t intensity); | ||
| core::Result reset(); | ||
|
|
||
| private: |
There was a problem hiding this comment.
don't need multiple private's
There was a problem hiding this comment.
at some point someone decided we have a second one separating variables from methods, idk if i like it tho
| core::Result reset(); | ||
|
|
||
| private: | ||
| LedDriver(core::ILogger &logger, std::shared_ptr<io::II2c> i2c); |
There was a problem hiding this comment.
Constructor should be public
There was a problem hiding this comment.
although I'm not sure it needs to be if it is only called from create
There was a problem hiding this comment.
i feel like it has had to be public even with the create, but i dont remembeer why. if it builds it builds
| logger.log(core::LogLevel::kFatal, "Failed to initialise LED driver"); | ||
| return std::nullopt; | ||
| } | ||
|
|
There was a problem hiding this comment.
remove the whitespace, same throughout the PR
4 participants
No description provided.