Skip to content

Conversation

amirgon
Copy link
Contributor

@amirgon amirgon commented Jun 15, 2021

Fixes and improvements to ffi callback

Add an optional 'lock' kwarg to callback that locks gc and scheduler.
This allows the callback to be invoked asynchronously in 'interrupt
context', for example as a signal handler

Added buffer protocol to callback that allows retrieving the C callback
function. This is needed when the callback should be set to a struct
field.

Usage example - Creating a signal handler and setting sa_handler to it:

    sa = new(sigaction_t)
    cb = ffi.callback("v", handler, "i", lock=True)
    sa.sa_handler = cb.cfun()

Related: #7373

(void)flags;
mp_obj_fficallback_t *self = MP_OBJ_TO_PTR(self_in);

bufinfo->buf = &self->func;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel like the right approach, using the buffer protocol to get out the C closure wrapper function (I would expect it to get access to the underlying data of the function itself).

Instead I'd suggest to add a method to the callback type, eg cfun(). Then one can write:

sa.sa_handler = cb.cfun()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

However, sa.sa_handler = cb.cfun() doesn't work in this case because we want to set the pointer value, not the value the pointer points at.

So we need something like this instead:

    memoryview(sa.sa_handler)[:] = cb.cfun()

Copy link
Member

Choose a reason for hiding this comment

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

However, sa.sa_handler = cb.cfun() doesn't work in this case because we want to set the pointer value, not the value the pointer points at.

If sa_handler were defined as a UINT64 would it work without the memoryview?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right.
I changed cfun to return ULL integer and defined sa.sa_handler as uctypes.UINT64, it works fine.

@amirgon amirgon force-pushed the micropython_pr/ffi_fixes branch 4 times, most recently from faca276 to 5144356 Compare June 18, 2021 12:16
@amirgon
Copy link
Contributor Author

amirgon commented Jun 18, 2021

I'm not sure why "coverage/coveralls" test fails.
It's doesn't seem to be related to modffi.c changes.

for (uint i = 0; i < cif->nargs; i++) {
pyargs[i] = mp_obj_new_int(*(mp_int_t *)args[i]);
}
mp_obj_t res = mp_call_function_n_kw(MP_OBJ_FROM_PTR(func), cif->nargs, 0, pyargs);
mp_obj_t res = mp_call_function_n_kw(pyfunc, cif->nargs, 0, pyargs);
Copy link
Member

Choose a reason for hiding this comment

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

If this call raises an exception, and locking is enabled, then things will not go well, the GC will remain locked and the code will fail completely.

I think it makes sense to have two separate callback functions: call_py_func and call_py_func_with_lock. The correct one is registered with ffi_prep_closure_loc. The wrapper call_py_func_with_lock will do something similar to lib/utils/mpirq.c:mp_irq_handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@amirgon amirgon force-pushed the micropython_pr/ffi_fixes branch 2 times, most recently from 4c6b493 to 125064b Compare June 20, 2021 21:46
Fixes and improvements to ffi callback

Added an optional 'lock' kwarg to callback that locks gc and scheduler.
This allows the callback to be invoked asynchronously in 'interrupt
context', for example as a signal handler.

Added 'cfun' member function to callback, that allows retrieving the C
callback function address.
This is needed when the callback should be set to a struct field.

Signed-off-by: Amir Gonnen <amirgonnen@gmail.com>
@amirgon amirgon force-pushed the micropython_pr/ffi_fixes branch from 125064b to 6c7ad06 Compare June 20, 2021 21:51
@dpgeorge
Copy link
Member

Thanks for updating. Merged in cb332dd with some minor edits (use mp_printf instead of printf).

@dpgeorge dpgeorge closed this Jun 24, 2021
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants