[WIP] UART #173
Draft
[WIP] UART #173
Conversation
This file contains 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
PerMalmberg
reviewed
Jan 25, 2023
| 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
Comment on lines
+389
to
+391
| 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.
Suggested change
| 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) |
There was a problem hiding this comment.
Please use positive logic as per contributing.md
|
|
||
| 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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.