Initial implementation of machine.Signal #2747

pfalcon · 2017-01-01T17:29:07Z

This implements a bare minimum of #2603 . machine.Signal is a cornerstone feature allowing to write portable applications interacting with hardware. As an immediate goal, it allows to fix currently broken (not working per specification) examples in examples/hwapi/ . An example of fix for soft_pwm.py for ESP8266 ESP-12 module is included - with it, LED fading happens as a smooth continuous wave, whereas currently it's discontinuous pulsations with blackouts.

dpgeorge

I see that you didn't go as far as making time_pulse_us() work with Signal objects. Doing that would require indirection for all mp_hal_pin_XXX method calls and make the software I2C and SPI slower (this could be noticeable in existing code, eg sending frames to a display would give a slower frame rate).

I also wanted to bring up the point that I think PinBase should be removed and instead we should use pure duck typing, ie if an object implements value() then it can be used wherever a Pin is required (and doesn't need to explicitly derive from PinBase). This point doesn't directly impact this PR, but it would require changing how the pin ioctl stuff works.

dpgeorge · 2017-01-10T02:48:14Z

esp8266/modmachine.c

@@ -245,6 +246,7 @@ STATIC const mp_rom_map_elem_t machine_module_globals_table[] = {
    { MP_ROM_QSTR(MP_QSTR_disable_irq), MP_ROM_PTR(&machine_disable_irq_obj) },
    { MP_ROM_QSTR(MP_QSTR_enable_irq), MP_ROM_PTR(&machine_enable_irq_obj) },

+    { MP_ROM_QSTR(MP_QSTR_Signal), MP_ROM_PTR(&machine_signal_type) },


I'd group it with the other types/classes below, not with the time_pulse_us function.

It was grouped by criteria of "generic vs port-specific", but ok, done.

dpgeorge · 2017-01-10T02:48:44Z

extmod/machine_signal.c

@@ -0,0 +1,118 @@
+#include <stdio.h>


Should be moved to below.

Removed debug header.

dpgeorge · 2017-01-10T02:50:45Z

extmod/machine_signal.c

+
+STATIC mp_obj_t signal_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *args) {
+    static const mp_arg_t allowed_args[] = {
+        { MP_QSTR_, MP_ARG_OBJ | MP_ARG_REQUIRED },


Why not name it "pin" so it doesn't give strange error messages if the argument is missing?

Because it's a positional parameter, so naming it wastes memory (without trying to imagine whether "pin" QSTR existed already, and will exist). An error message won't be more strange than for other functions which rightfully use guaranteedly existing empty QSTR.

dpgeorge · 2017-01-10T02:52:45Z

extmod/machine_signal.c

+    struct {
+        mp_arg_val_t pin;
+        mp_arg_val_t inverted;
+    } parsed_args;


Passing address of this doesn't guarantee that it can be interpreted as an array of mp_arg_val_t's. Instead better to use an array combined with enums, see, eg, machine_i2c_obj_init_helper.

I remember that discussion and I kinda didn't agree with it, that it makes sense to go thru extra hops given that member of a structure are naturally aligned. There're other cases of using a structure in the codebase.

Well, it's the C standard you don't agree with then. And there are many more uses of the enum in the code base so we should stick with that (git grep "enum { ARG_").

The C standard talks about the alignment and the alignment, it doesn't say that they has to be different. The C standard probably doesn't guarantee user's access to preprocessor's output, and yet we rely on that.

And there are many more uses of the enum in the code base so we should stick with that (git grep "enum { ARG_").

Maybe. But then need to consistently convert all the cases. Then perhaps this case can be left until then too?

dpgeorge · 2017-01-10T02:57:28Z

extmod/machine_signal.c

+    mp_obj_base_t base;
+    mp_obj_t pin;
+    bool inverted;
+} mp_signal_t;


Would it be better to name it mp_machine_signal_t (or even machine_signal_t) so not to confuse with a possible internal signal mechanism of the core (like unix signals)?

dpgeorge · 2017-01-10T03:17:55Z

extmod/machine_signal.c

+    mp_arg_check_num(n_args, n_kw, 0, 1, false);
+    if (n_args == 0) {
+        // get pin
+        return MP_OBJ_NEW_SMALL_INT(mp_virtual_pin_read(self_in));


This is going to be slow.... why not do the inversion here and call virtual_pin_read on self->pin?

Perhaps this was done to save code size, or just as a copy-paste of existing code. Optimizations can be left for later.

pfalcon · 2017-01-10T06:45:45Z

I see that you didn't go as far as making time_pulse_us() work with Signal objects. Doing that would require indirection for all mp_hal_pin_XXX method calls and make the software I2C and SPI slower (this could be noticeable in existing code, eg sending frames to a display would give a slower frame rate).

Well, this is initial step of the implementation, and it doesn't implement all ideas discussed in #2603. And I do keep in mind that bitbanging SPI/I2C needs to be fast. Then at the introduction of PinBase we agreed that mp_hal_pin_read/write() can be implemented per-port either as using generic polymorphic mp_hal_pin_read(), etc., or using fast port-adhoc versions. So, looking forward, we'd need to split it up into 2 API layers, by doing the same trick as you did with mp_hal_delay_us_fast() - mp_hal_pin_read() would guaranteedly support virtual pins (and thus Signals) and mp_hal_pin_read_fast() won't support Signals.

I also wanted to bring up the point that I think PinBase should be removed and instead we should use pure duck typing, ie if an object implements value() then it can be used wherever a Pin is required (and doesn't need to explicitly derive from PinBase). This point doesn't directly impact this PR, but it would require changing how the pin ioctl stuff works.

Well, that's not how it works. To make virtual pin API as fast as possible, it's a single indirect function call from a pointer stored in a structure: o->type->protocol->ioctl(). No if's and stuff. PinBase is a mechanism which allows Python-level code to adhere to the same process. Trying to remove it will make it only slower. We would use more, not less of the idea used by PinBase. For example, if we'd ever want to make stream classes implementable from Python, they would need to inherit from io.RawIOBase, etc. (exactly like CPython mandates it), containing similar adapters (I don't think that it needs to be implemented, but if ever want to make (more of) CPython testsuite to pass, that's the way to do it.)

dpgeorge · 2017-01-16T13:28:52Z

To make virtual pin API as fast as possible, it's a single indirect function call from a pointer stored in a structure: o->type->protocol->ioctl()

It can still be a single call. When accessing an object that you want to use as a pin (or stream, or block protocol for mounting, etc) the first thing you need to do is check that it has the required functionality. This is generally done "once" the first time the object is accessed. This check can check if it is native, or duck-typed. In the latter case it just returns an adaptor object that turns the o->type->protocol->ioctl() call into an underlying call to the relevant Python method.

Note that this checking whether an object has the required functionality is needed so that you don't get crashes with, eg, passing a pin where a stream is expected.

Using pure duck-typing (with no PinBase etc) also allows a single object to implement multiple protocols (not possible with subclassing native because you can't derive from multiple native classes). And this would be desirable, eg a custom pin object that can be selected/polled (needs both pin protocol and stream/poll protocol).

pfalcon · 2017-01-29T00:15:43Z

This check can check if it is native, or duck-typed. In the latter case it just returns an adaptor object that turns the o->type->protocol->ioctl() call into an underlying call to the relevant Python method.

I'm not sure I follow. It needs to be stored somewhere. Do you mean, in addition to mp_obj_t pin, also store ioctl vmethod ptr? That's storing more. Here we have space, but in general...

Using pure duck-typing (with no PinBase etc)

Anyway, PinBase exists in the codebase, and this patch adds single new feature, using what exists in the codebase. If there's an idea how to get rid of PinBase without losing anything and even gaining something, it's still a separate idea.

dpgeorge · 2017-01-29T12:37:22Z

extmod/machine_signal.c

+    struct {
+        mp_arg_val_t pin;
+        mp_arg_val_t inverted;
+    } parsed_args;


Well, it's the C standard you don't agree with then. And there are many more uses of the enum in the code base so we should stick with that (git grep "enum { ARG_").

dpgeorge · 2017-01-29T12:37:59Z

esp8266/machine_pin.c

@@ -374,6 +375,23 @@ STATIC mp_obj_t pyb_pin_irq(size_t n_args, const mp_obj_t *pos_args, mp_map_t *k
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_KW(pyb_pin_irq_obj, 1, pyb_pin_irq);

+STATIC mp_uint_t pin_ioctl(mp_obj_t self_in, mp_uint_t request, uintptr_t arg, int *errcode);


Is there a reason for these declarations? They are static functions so there shouldn't be any compiler warning without them.

There should not be, but apparently, there were, thanks to over-zealous compilation warnings-errors in some builds.

dpgeorge · 2017-01-29T12:38:23Z

extmod/machine_signal.c

+    return MP_OBJ_FROM_PTR(o);
+}
+
+STATIC mp_uint_t signal_ioctl(mp_obj_t self_in, mp_uint_t request, uintptr_t arg, int *errcode);


Extra declaration.

dpgeorge · 2017-01-29T12:41:55Z

If there's an idea how to get rid of PinBase without losing anything and even gaining something, it's still a separate idea.

If I get time I'll make up a proof-of-concept patch.

pfalcon · 2017-01-29T13:09:36Z

If I get time I'll make up a proof-of-concept patch.

Maybe it's worth to open an RFC with description of how it would work to make sure it scales beyond just PinBase, e.g. to stream-in-python case mentioned above.

For polymorphic interfacing on C level.

…ule. A signal is like a pin, but ca also be inverted (active low). As such, it abstracts properties of various physical devices, like LEDs, buttons, relays, buzzers, etc. To instantiate a Signal: pin = machine.Pin(...) signal = machine.Signal(pin, inverted=True) signal has the same .value() and __call__() methods as a pin.

pfalcon · 2017-01-29T16:05:30Z

Ok, fixed remaining concerns (array/struct, extra prototype) and merged. Thanks.

kfricke · 2017-01-29T17:15:04Z

This PR and the surrounding RFC discussion is really a good inspiration and lesson. Not that i can follow you both into all those details, but thanks anyways!

Besides that it is quite annoying to learn from these discussions the steep and stony way, just because one of the two maintainers of MicroPython has blocked you on Github completely.

dpgeorge · 2017-01-30T13:10:30Z

Maybe it's worth to open an RFC with description of how it would work to make sure it scales beyond just PinBase, e.g. to stream-in-python case mentioned above.

See #2824 (it was in my git stash).

mimxrt1011: Only re-init SPI when it's actually needed

pfalcon force-pushed the machine-signal branch from 8da3ac8 to bd9ba33 Compare January 4, 2017 10:12

dpgeorge reviewed Jan 10, 2017

View reviewed changes

pfalcon force-pushed the master branch from 91ecff0 to 56e7ebf Compare January 28, 2017 09:08

pfalcon force-pushed the machine-signal branch from bd9ba33 to 01b9db3 Compare January 28, 2017 23:57

dpgeorge reviewed Jan 29, 2017

View reviewed changes

Paul Sokolovsky added 4 commits January 29, 2017 18:47

esp8266/machine_pin: Implement pin ioctl protocol.

18b6835

For polymorphic interfacing on C level.

esp8266/modmachine: Add Signal class.

287180a

examples/hwapi: Use Signal for inverted LED on ESP-12.

297af60

pfalcon force-pushed the machine-signal branch from 01b9db3 to 297af60 Compare January 29, 2017 15:57

pfalcon merged commit 297af60 into micropython:master Jan 29, 2017

dpgeorge mentioned this pull request Jan 30, 2017

RFC: py/stream: Add user-streams adaptor. #2824

Closed

tannewt added a commit to tannewt/circuitpython that referenced this pull request Apr 7, 2020

Merge pull request micropython#2747 from arturo182/imx-spi-init-check

2900d92

mimxrt1011: Only re-init SPI when it's actually needed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Initial implementation of machine.Signal #2747

Initial implementation of machine.Signal #2747

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Initial implementation of machine.Signal #2747

Initial implementation of machine.Signal #2747

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!