feature: shift register #458
Conversation
|
i made the PR draft. when the PR passes the tests, please remove please consolidate commits, especially when you reverted commits, by (so far, looks good) |
JaimeAlbq
force-pushed
the
feature-shift-reg
branch
2 times, most recently
from
01ae8f9
to
39471fb
Compare
|
here is my first review. the biggest one is the order of arguments. all in all, a good PR. |
No problem. If you need anything from my side or a hand, I'll be there. |
The feature is a driver to interface the microcontroller with shift registers just using 3 pins: Clock, Data and Latch. For further information, please, take a look at the component [README.md](components/shift_reg/README.md). Feature requested by issue UncleRus#457.
JaimeAlbq
force-pushed
the
feature-shift-reg
branch
from
39471fb
to
5134fda
Compare
|
Hello @trombik, Could you please perform the review one more time. I did the required changes. |
|
@JaimeAlbq will do after the weekend (I work on weekends) |
components/shift_reg/shift_reg.c
Outdated
| // MSB Mode | ||
| for (int8_t i = 7; i >= 0; i--) | ||
| { | ||
| if ((data >> i) & 1) |
There was a problem hiding this comment.
This condition can be replaced by a single expression:
gpio_set_level(dev->pin.data, (data >> i) & 1);
|
|
||
| esp_err_t shift_reg_latch(shift_reg_config_t *dev) | ||
| { | ||
| gpio_set_level(dev->pin.latch, true); |
There was a problem hiding this comment.
Here we need to check if dev is NULL
| #include <driver/gpio.h> | ||
| #include <ets_sys.h> | ||
|
|
||
| typedef enum { |
There was a problem hiding this comment.
All structs and enums must be commented in doxy-style, like this:
/**
* This structure is intended to store the device handle and its state.
*/
typedef struct {
int foo; //< Special field
} some_device_t;
...
/**
* Device operational modes
*/
typedef enum {
MODE_FOO = 0; //< Mode Foo
MODE_BAR; //< Mode Bar
} some_mode_t;
| @@ -0,0 +1 @@ | |||
| # add required non-default option for the example if any | |||
There was a problem hiding this comment.
Please delete this file if the example does not require special ESP-IDF settings.
| ESP_ERROR_CHECK(shift_reg_send(&shifter, value, 2)); | ||
| ESP_ERROR_CHECK(shift_reg_latch(&shifter)); | ||
| ESP_LOGI(__func__, "reg_value: 0x%02X 0x%02X", shifter.reg_value[0], shifter.reg_value[1]); | ||
| vTaskDelay(2 / portTICK_PERIOD_MS); |
There was a problem hiding this comment.
use pdMS_TO_TICKS instead of portTICK_PERIOD_MS which is deprecated.
components/shift_reg/README.md
Outdated
| To initialize the shift register it is going to need to call the `esp_err_t shift_reg_init(shift_reg_config_t *dev)`, by passing the created shift register configurations. | ||
|
|
||
| ### De-initialization | ||
| Since the `uint8_t *reg_value` is created accordingly of the number of registers, the vector allocate the necessary size in the heap memory, so `esp_err_t shift_reg_deinit(shift_reg_config_t *dev)` can be used to free this memory. |
There was a problem hiding this comment.
this should go to the header file so that users can see this in the official documentation. see other documentation pages under docs/source/groups.
|
@JaimeAlbq please read the review comments, and modify the code, or justify. after that, I'll test the code on real hardware. |
- `examples/shift_reg/default/sdkconfig.defaults`: removed - `README.md`: moved and renamed to `docs/source/groups/shift_reg.rst` - `examples/shift_reg/default/main/main.c`: updated the `portTICK_PERIOD_MS` to `pdMS_TO_TICKS` - `components/shift_reg/shift_reg.h`: added comment in doxygen-style for structures and enums - `components/shift_reg/shift_reg.c`: - `shift_reg_latch()`: added argument checker for `NULL` pointer - `shift_reg_send8bits()`: improved the GPIO set
`.eil.yml`: updated
`.eil.yml`: updated
|
Call devtools.py from devtools directory with arguments check. If check is okay then call with argument render. There are some doc files are missing and the tool should help you to generate them. |
|
I did a small check on your branch. You should update the branch to the current master version. Then first you have to add your name to the devtools/persons.yml. Then you have to remove 'name:' from depends section in your .eil.yml After you have made the changes run devtooly.py check and devtooly.py render again. |
shift_reg: add shift_reg component
The feature is a driver to interface the microcontroller with shift
registers just using 3 pins: Clock, Data and Latch.
For further information, please, take a look at the component
README.md.
Feature requested by issue #457.