Conversation
PerMalmberg
left a comment
PerMalmberg
left a comment
There was a problem hiding this comment.
Hello!
What's the plan with this class, except wrapping IDF functions into a class with added logging?
| UART::UART(uart_port_t port, int baud_rate, uart_word_length_t data_bits, uart_parity_t parity, uart_stop_bits_t stop_bits, uart_hw_flowcontrol_t flow_control, gpio_num_t tx_pin, gpio_num_t rx_pin, gpio_num_t rts_pin, gpio_num_t cts_pin, uart_mode_t mode) | ||
| : port_(port) | ||
| { | ||
| this->config_ = uart_config_t{ |
There was a problem hiding this comment.
No need for this-> and also from The contributing.md document:
Do not use Hungarian notation for variables, i.e. warts m_
| void UART::log_error(const std::string msg, esp_err_t err) | ||
| { | ||
| std::stringstream ss; | ||
| ss << "UART Error for port '" << static_cast<int>(port_) << "', " << msg << ": " << esp_err_to_name(err); |
There was a problem hiding this comment.
You never print out the error. There's also this: https://github.com/PerMalmberg/Smooth/blob/master/lib/smooth/include/smooth/core/logging/log.h
| uart_port_t port_; | ||
|
|
||
| uart_config_t config_; |
There was a problem hiding this comment.
as above, no warts please :)
| /// Log an error message. | ||
| /// \param msg The error message | ||
| /// \param err The error code | ||
| void log_error(const std::string msg, esp_err_t err); |
There was a problem hiding this comment.
| void log_error(const std::string msg, esp_err_t err); | |
| void log_error(const std::string& msg, esp_err_t err); |
No reason to pass this function a copy of the string, no use of move-semantics that I can see?
| bool UART::get_word_length(uart_word_length_t &data_bit) | ||
| { | ||
| const auto res = uart_get_word_length(this->port_, &data_bit); | ||
| if(res != ESP_OK) |
|
|
||
| bool UART::set_word_length(const uart_word_length_t &data_bit) | ||
| { | ||
| auto res = uart_set_word_length(this->port_, data_bit); |
There was a problem hiding this comment.
You're meticulous with const elsewhere, but missed this one.
2 participants
Not ready for merge yet. I am just adding it already now, so that I can get feedback as soon as possible and don't have to refactor later.
I need this to support a finger print sensor, for which I will add code later.