port_driver_destroy_cb is never called and only appears in tests

[jgerrish]

port_driver_destroy_cb is a field of PortDriverDef in portnifloader.h that is set to the DESTROY_CB parameter by the REGISTER_PORT_DRIVER macro.

The field is used by the port_driver_destroy_all functions in src/libAtomVM/portnifloader.c and src/platforms/rp2/src/lib/sys.c

port_driver_driver_destroy_all is only called in src/platforms/esp32/test/main/test_main.c

It is never called in actual code.

I believe it is meant to do global cleanup of all the instances or ports of the driver.

Very few of the drivers in the esp32 tree use the destroy callback, but it is still used in a few drivers such as socket_driver.c

My driver can do cleanup with a "close" mailbox action, but I still think a driver-wide cleanup like you have in your design makes sense in case there is any other driver-wide shutdown needed or as a fallback to get the hardware to a sane state if mailbox messages aren't delivered.

test_main.c shows a pattern for how it could be inserted into src/platforms/esp32/main.c:app_main() at the end.
The RP2 tree also may need to call it in src/platforms/rp2/src/main.c:app_main()

nif_collection_destroy_all isn't called in the ESP32 tree either, but it is called in RP2.

Or maybe I'm missing something and these functions are called elsewhere.

It looks like a possibly smart API design sign, just a mistake in not calling it. I can write a PR to add those missing calls if needed, let me know. It might need a lot of testing since it is an unused code path that some drivers implement but haven't tested.

[jgerrish]

If leaving those callbacks unhooked up was a deliberate choice, I understand that too.

I guess that would be going with the Erlang philosophy of "Let it crash" and delegating device recovery to the hardware level. There are power-management and other costs to that, but it does encourage systems thinking and exercising hardware watchdogs and hardware management reset design.

I figured the architecture layer for "Let it crash" was a level up, with AtomVM / Erlang / BEAM apps, but either way works.

But in that case we should document that the callbacks aren't called, so developers don't rely on them accidentally.

Thanks.

[petermm]

I'm no expert - but my impression is that for esp32 the underlying esp-idf doesn't have a strong focus on supporting complete and correct teardown of a driver.

I do think things have gotten better, but they are still far from perfect - so that's one reason, another is our developer resources, supporting changing APIs etc, and yet another is flash size..

but imho - mostly it's the lack of focus on proper teardown and memory recovery from esp-idf SDK - in the perfect world we had those teardowns, and they would also be working - maybe it's worth revisiting with esp-idf 6 - their SDK is becoming very stable.

[jgerrish]

Ok, I understand if that's the call right now. The project has a large amount of development going on right now, and I understand it's not a priority. It's fantastic seeing all the new features and work on testing every week.

I'm using it in an ESP32 i2s driver to make my AtomVM layer as reliable as possible, and for extra metrics on failure paths (It's easy for a developer to forget a close call on the port, etc.) but I can feature-gate it with an #ifdef for now.

The ESP32 socket driver (components/avm_builtins/socket_driver.c) currently uses the destroy callback too. The authors should be contacted or a PR created or something to make sure they understand it isn't hooked up. They use malloc to allocate memory for a socket listener in the socket_driver_init and free it in a socket_driver_destoy callback. If you're just defaulting to hard reset the hardware on AtomVM reboot or failure, it should be ok, but the main AtomVM process flow can change too to and I know there are config options around reboot that might interact with that.

And the getting-started documentation should be updated to indicate it isn't called.