Skip to content

Enforces more consistency into Peripheral Manager #8188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 11, 2023
Merged

Enforces more consistency into Peripheral Manager #8188

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
448c8d8
Avoid log_i() message the first time a bus is assigned
SuGlider May 10, 2023
807f2c9
Prevent operation with ESP32_BUS_TYPE_INIT
SuGlider May 10, 2023
8ca8b97
keeps coding style
SuGlider May 10, 2023
a97ca62
do not print messages on INIT bus type
SuGlider May 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions cores/esp32/esp32-hal-periman.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,16 @@ bool perimanSetPinBus(uint8_t pin, peripheral_bus_type_t type, void * bus){
log_e("Bus is NULL");
return false;
}
if (type == ESP32_BUS_TYPE_INIT && bus != NULL){
log_e("Can't set a Bus to INIT Type");
return false;
}
otype = pins[pin].type;
obus = pins[pin].bus;
if(type == otype && bus == obus){
log_i("Bus already set");
if (type != ESP32_BUS_TYPE_INIT) {
log_i("Bus already set");
}
return true;
}
if(obus != NULL){
Expand All @@ -59,7 +65,7 @@ void * perimanGetPinBus(uint8_t pin, peripheral_bus_type_t type){
log_e("Invalid pin: %u", pin);
return NULL;
}
if(type >= ESP32_BUS_TYPE_MAX){
if(type >= ESP32_BUS_TYPE_MAX || type == ESP32_BUS_TYPE_INIT){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not an invalid call per say. It will always return NULL. No need to print an error really and remember that peripheral manager is internal API

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the code clear about it. As commented below.

The added testing for type == ESP32_BUS_TYPE_INIT is inteded to make it clear in the code. We shall think that today we are working on it and know that we won't use it with ESP32_BUS_TYPE_INIT, but in the future, other people may be working on this code.

I think that all that makes the code clearer for others, including commentaries, shall be used.

log_e("Invalid type: %u", (unsigned int)type);
return NULL;
}
Expand All @@ -78,7 +84,7 @@ peripheral_bus_type_t perimanGetPinBusType(uint8_t pin){
}

bool perimanSetBusDeinit(peripheral_bus_type_t type, peripheral_bus_deinit_cb_t cb){
if(type >= ESP32_BUS_TYPE_MAX){
if(type >= ESP32_BUS_TYPE_MAX || type == ESP32_BUS_TYPE_INIT){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as previous comment from @me-no-dev, we will never call perimanSetBusDeinit with INIT type. This check is not that necessary :) but of course is not breaking anything and can be there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the code clear about it. As commented below.

The added testing for type == ESP32_BUS_TYPE_INIT is inteded to make it clear in the code. We shall think that today we are working on it and know that we won't use it with ESP32_BUS_TYPE_INIT, but in the future, other people may be working on this code.

I think that all that makes the code clearer for others, including commentaries, shall be used.

log_e("Invalid type: %u", (unsigned int)type);
return false;
}
Expand Down