Skip to content

Fix bug in ESP32 gpio:stop/0 and gpio:close/1 #1057

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 2 commits into from
Mar 3, 2024
Merged

Fix bug in ESP32 gpio:stop/0 and gpio:close/1 #1057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5bacbb8
Add missing memory check to ESP32 GPIO port driver
UncleGrumpy Feb 21, 2024
b61cc87
Fix crash when gpio:stop/0 or gpio:close/1 are used on ESP32
UncleGrumpy Feb 23, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- ESP32: fix i2c_driver_acquire and i2c_driver_release functions, that were working only once.
- Sending messages to registered processes using the `!` operator now works.
- Fixed bug in `OP_SEND` that would accept sending a message to any integer or term without raising an error.
- ESP32: fixed bug in `gpio:stop/0` and `gpio:close/1` that would cause the VM to crash.

## [0.6.0-beta.0] - 2024-02-08

Expand Down
8 changes: 7 additions & 1 deletion src/platforms/esp32/components/avm_builtins/gpio_driver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,11 @@ Context *gpio_driver_create_port(GlobalContext *global, term opts)
Context *ctx = context_new(global);

struct GPIOData *gpio_data = malloc(sizeof(struct GPIOData));
if (IS_NULL_PTR(gpio_data)) {
ESP_LOGE(TAG, "Not enough free memory to initialize GPIO port driver!");
scheduler_terminate(ctx);
return NULL;
}
list_init(&gpio_data->gpio_listeners);

ctx->native_handler = consume_gpio_mailbox;
Expand Down Expand Up @@ -270,7 +275,7 @@ static term gpiodriver_close(Context *ctx)
struct GPIOListenerData *gpio_listener = GET_LIST_ENTRY(item, struct GPIOListenerData, gpio_listener_list_head);
gpio_num = gpio_listener->gpio;
list_remove(&gpio_listener->gpio_listener_list_head);
list_remove(&gpio_listener->listener.listeners_list_head);
sys_unregister_listener(ctx->global, &gpio_listener->listener);
free(gpio_listener);

gpio_set_intr_type(gpio_num, GPIO_INTR_DISABLE);
Expand All @@ -282,6 +287,7 @@ static term gpiodriver_close(Context *ctx)
}
}

ctx->platform_data = NULL;
globalcontext_unregister_process(glb, gpio_atom_index);
free(gpio_data);
Copy link
Collaborator

@fadushin fadushin Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is free-ing platform_data and nullifying the reference supposed to be done here or should it be deferred to context_destroy. Perhaps @pguyot should weigh in on best practices here. There is the reference in the context_destroy comments about interrupts possibly getting a handle on the context, which could be relevant here, since we have a queue attached to this process, but I haven't studied it in detail.

Copy link
Collaborator Author

@UncleGrumpy UncleGrumpy Feb 22, 2024

Choose a reason for hiding this comment

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

I would like to hear what @pguyot has to say. This is also what is done in the I2C driver, nullifying the reference was very recently added, which pointed me in the direction of using this, and does fix the crash described in #816.

Copy link
Collaborator

@fadushin fadushin Feb 23, 2024

Choose a reason for hiding this comment

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

In the absence of a response, I say we merge this in. The context_destroy function is defensive about whether to delete platform data if it is already null, so we don’t have to worry about double deletes.

IMHO this module needs to be re-written as a platform Nif so that it will automatically close on a GC after the last reference goes out of scope, but that is a different kettle.

Copy link
Collaborator

@pguyot pguyot Feb 23, 2024

Choose a reason for hiding this comment

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

I believe this change is correct but unsufficient. If platform_data is freed (as it currently is in gpiodriver_close), it should be nullified as indeed context_destroy will attempt to free it again if it is not null, hence crash #816.

The reason the free was added in context_destroy is to avoid any memory leak. And free is called at the end of the destroy function, after the process is made unreachable (is removed from the process table), to cope with cases where the platform data is called from interruptions. This is typically the "ugly hack" mentioned in i2c driver.

It looks like there still is a problem with GPIO driver, though. gpiodriver_close loops over listeners like gpiodriver_remove_int, but doesn't do it as well. It does call gpio_isr_handler_remove, but it doesn't call sys_unregister_listener unlike gpiodriver_remove_int.

Copy link
Collaborator Author

@UncleGrumpy UncleGrumpy Feb 23, 2024

Choose a reason for hiding this comment

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

Indeed. I was unsafely removing the listener.listeners_list_head manually (without the lock), rather than using the safe sys_unregister_listener

Copy link
Collaborator Author

@UncleGrumpy UncleGrumpy Feb 24, 2024

Choose a reason for hiding this comment

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

This is fixed.

Copy link
Collaborator

@pguyot pguyot Mar 3, 2024

Choose a reason for hiding this comment

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

We have 9 duplicate lines. Do you think we should factorize them?
277-286 and 457-466.

Copy link
Collaborator Author

@UncleGrumpy UncleGrumpy Mar 3, 2024

Choose a reason for hiding this comment

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

Refactoring may be a good idea, it could alleviate future discrepancies between the gpiodriver_close and gpiodriver_remove_int handling of removing the listener.

If I were to replace the duplicate code with a common remove_gpio_listener function would it make sense to make that an inline function in this case?

Copy link
Collaborator

@pguyot pguyot Mar 3, 2024

Choose a reason for hiding this comment

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

Of course the factorized code can be inlined.


Expand Down