Skip to content
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.

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

First draft of eveL, the low-level module of the Gameduino bindings #2581

Merged
merged 15 commits into from
Feb 13, 2020
Merged

First draft of eveL, the low-level module of the Gameduino bindings #2581

merged 15 commits into from
Feb 13, 2020

Conversation

jamesbowman
Copy link

First draft of eveL, the low-level module of the Gameduino (and BridgeTek EVE) bindings.

[#2578]

@jamesbowman
Copy link
Author

Smoke test for the built module:

import array

from eveL import EVEL

class Dumper(EVEL):
    def write(self, b):
        f.bb += b

f = Dumper()
f.register(f)
f.bb = b''
f.ClearColorRGB(0x12, 0x34, 0x56)
f.flush()
assert list(array.array("I", f.bb)) == [0x02123456], "Unexpected output %r" % f.bb
print("eveL smoke test passed")

Also at https://github.com/jamesbowman/py-bteve/blob/master/examples/smoke_eveL.py

Exclude modeveL-gen.h from Sphinx build
Fix build break because of overflowing small ports
@jamesbowman
Copy link
Author

I'm debugging a MicroPython vs CircuitPython difference right now. This causes crashes on CircuitPython. Will update here when I understand more.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you for this first draft! I've replied with a couple high level questions.

shared-bindings/eveL/__init__.c Outdated Show resolved Hide resolved

// #if MICROPY_PY_BUILTINS_EVEL

//| :mod:`eveL` --- low-level BridgeTek EVE bindings
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend calling this _eve. The _ prefix in Python denotes that it is private and the api may change without warning. Then you can call the high level library eve.

}
STATIC MP_DEFINE_CONST_FUN_OBJ_3(vertex2f_obj, _vertex2f);

STATIC mp_obj_t _cmd(size_t n_args, const mp_obj_t *args) {
Copy link
Member

Choose a reason for hiding this comment

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

How is this different than struct? It'd be great to separate the implementation of these functions from the definition and arg parsing so that they are better documented. Right now, it is unclear what the _eve module would contain.

Copy link
Author

Choose a reason for hiding this comment

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

  • How is this different than struct?

The methods in this module (e.g. _vertex2f) align their arguments on bit boundaries, not the byte boundaries used by struct. Also these methods append the binary commands to an internal buffer, avoiding the overhead of constructing an ephemeral bytes object for the binary command.

  • It'd be great to separate the implementation of these functions from the definition and arg parsing so that they are better documented.

So taking vertex2f, which currently looks like this:

STATIC mp_obj_t _vertex2f(mp_obj_t self , mp_obj_t a0, mp_obj_t a1) {
    int16_t x = (int16_t)(16 * mp_obj_get_float(a0));
    int16_t y = (int16_t)(16 * mp_obj_get_float(a1));
    C4(self, (0x40000000 | ((x & 32767) << 15) | (y & 32767)));
    return mp_const_none;
}
STATIC MP_DEFINE_CONST_FUN_OBJ_3(vertex2f_obj, _vertex2f);

Then I guess a split would be:

STATIC void vertex2f(mp_obj_t self, int16_x, int16_y) {
    C4(self, (0x40000000 | ((x & 32767) << 15) | (y & 32767)));
}

STATIC mp_obj_t _vertex2f(mp_obj_t self, mp_obj_t a0, mp_obj_t a1) {
    vertex2f(self,
             (int16_t)(16 * mp_obj_get_float(a0)),
             (int16_t)(16 * mp_obj_get_float(a1)));
    return mp_const_none;
}
STATIC MP_DEFINE_CONST_FUN_OBJ_3(vertex2f_obj, _vertex2f);

Then vertex2f is in the business of packing integer bitfields for the hardware.
And _vertex2f is in the business of arg parsing before handing off to the integer interface.

(And similarly for all the other C methods.)

Is this along the right lines?

  • Right now, it is unclear what the _eve module would contain.

Modules eve and _eve together contain all the command definitions that are in the hardware datasheet. The split is that _eve contains the parts that benefit most from being coded in C.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Ok, makes sense to use this for sub-byte packing. Note that struct can skip bytes objects with pack_into where you give it a bytearray.

  2. Yup, that is on the right track. We then put the arg parsing in shared-bindings and the implementation in shared-modules. shared-bindings can also have inline docs. We name everything verbosely too so it includes the module name (_eve in this case). The example implementation function would be common_hal__eve_vertex2f. The arg parsing methods can be shorter because they are internal to the object.

  3. Cool, sounds like the right structure then. It'd be great to document the C implementations for consistency with everything else.

@jamesbowman
Copy link
Author

@tannewt thanks for these - I'll get started on them later today.

For now I am stuck on what I think is an internals difference between MicroPython and CircuitPython. I wonder if you have any ideas on what might be going on.

I'm using this reduced module for testing, which makes a class EVEL with one method, "flush". The problem is that when EVEL is subclassed, the "self" that flush receives is not an EVEL.
Demo case:

from eveL import EVEL
    
print('works fine: EVEL')
    
s = EVEL()
s.flush()

print('not fine: EVEL subclassed')

class Foo(EVEL):
    pass
    
t = Foo()
t.flush()

Gives:

works fine: EVEL
EVEL make_new 20003370
EVEL flush (20003370)
not fine: EVEL subclassed
EVEL make_new 20003590
EVEL flush (20002df0)

This seems very strange. The C code for "flush()" now cannot access the private elements of EVEL because it's received a Foo instead.

Running the same code on MicroPython behaves as expected: flush() receives the same object that make_new made. From the unix port, same C code with minor tweaks:

EVEL make_new 7fd6790a34c0
EVEL flush (7fd6790a34c0)
not fine: EVEL subclassed
EVEL make_new 7fd6790a3ae0
EVEL flush (7fd6790a3ae0)

The minimal module that shows the problem:

typedef struct _mp_obj_EVEL_t {
    mp_obj_base_t base;
    mp_obj_t dest[3];
    size_t n;
    uint8_t buf[512];
} mp_obj_EVEL_t;

STATIC const mp_obj_type_t EVEL_type;

STATIC mp_obj_t _flush(mp_obj_t self) {
    mp_printf(&mp_plat_print, "EVEL flush (%p)\n", self);
    return mp_const_none;
}
STATIC MP_DEFINE_CONST_FUN_OBJ_1(flush_obj, _flush);

STATIC const mp_rom_map_elem_t EVEL_locals_dict_table[] = {
    { MP_ROM_QSTR(MP_QSTR_flush), MP_ROM_PTR(&flush_obj) }
};
STATIC MP_DEFINE_CONST_DICT(EVEL_locals_dict, EVEL_locals_dict_table);

STATIC void EVEL_print(const mp_print_t *print, mp_obj_t self_in, mp_print_kind_t kind) {
    (void)self_in;
    (void)kind;
    mp_printf(print, "<EVEL>");
}

STATIC mp_obj_t EVEL_make_new(const mp_obj_type_t *type, size_t n_args, const mp_obj_t *args, mp_map_t *kw_args) {
    // mp_arg_check_num(n_args, kw_args, 1, 1, false);
    mp_obj_EVEL_t *o = m_new_obj(mp_obj_EVEL_t);
    mp_printf(&mp_plat_print, "EVEL make_new %p\n", o);
    o->base.type = &EVEL_type;
    return MP_OBJ_FROM_PTR(o);
}

STATIC const mp_obj_type_t EVEL_type = {
    { &mp_type_type },
    // Save on qstr's, reuse same as for module
    .name = MP_QSTR_EVEL,
    .print = EVEL_print,
    .make_new = EVEL_make_new,
    .locals_dict = (void*)&EVEL_locals_dict,
};

STATIC const mp_rom_map_elem_t mp_module_eveL_globals_table[] = {
    { MP_ROM_QSTR(MP_QSTR___name__), MP_ROM_QSTR(MP_QSTR_eveL) },
    { MP_ROM_QSTR(MP_QSTR_EVEL), MP_OBJ_FROM_PTR(&EVEL_type) },
};

STATIC MP_DEFINE_CONST_DICT(mp_module_eveL_globals, mp_module_eveL_globals_table);

const mp_obj_module_t eveL_module = {
    .base = { &mp_type_module },
    .globals = (mp_obj_dict_t*)&mp_module_eveL_globals,
};

@tannewt
Copy link
Member

tannewt commented Feb 5, 2020

Yes, this is a deliberate difference. We want native code to be able to call functions on a subclass. (_pixelbuf needs this) To get the native struct do something like: mp_obj_t native_pixelbuf = mp_instance_cast_to_native_base(pixelbuf_obj, &pixelbuf_pixelbuf_type); (from here)

@jamesbowman
Copy link
Author

In fact I can demonstrate the same problem the existing socket.socket class - here:

Adafruit CircuitPython 5.0.0-beta.4-130-g8347f2b5c-dirty on 2020-02-05; Adafruit Metro M4 Express with samd51j19
>>> 
>>> import socket
>>> u = socket.socket()
>>> u.settimeout(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 128] ENOTCONN
>>> 
>>> 
>>> 
>>> class Bar(socket.socket):
...     pass
...     
...     
... 
>>> v = Bar()
>>> v.settimeout(1)

causes a crash, because the C code for settimeout() got a "self" that is a Bar instead of a mod_network_socket_obj_t.

Again, this works as expected on MicroPython:

>>> import socket
>>> u=socket.socket()
>>> u
<_socket 4>
>>> u.settimeout(1)
>>> class Bar(socket.socket): pass
... 
>>> 
>>> v = Bar()
>>> v
<_socket 5>
>>> v.settimeout(1)
>>>

@jamesbowman
Copy link
Author

Aha, that's what I needed. Thanks.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you for the split! It's definitely getting closer! Would it be possible to merge all of auto-generated functions into init.c as well? I realize that would make updating a bit more work but it'd be more consistent here. Thanks!

shared-module/_eve/__init__.h Outdated Show resolved Hide resolved
shared-bindings/_eve/__init__.c Show resolved Hide resolved
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Getting really close! Thank you for the doc strings!

conf.py Outdated Show resolved Hide resolved
shared-bindings/_eve/__init__.c Show resolved Hide resolved
shared-bindings/_eve/__init__.c Outdated Show resolved Hide resolved
shared-bindings/_eve/__init__.c Outdated Show resolved Hide resolved
shared-bindings/_eve/__init__.c Outdated Show resolved Hide resolved

STATIC const mp_rom_map_elem_t mp_module__eve_globals_table[] = {
{ MP_ROM_QSTR(MP_QSTR___name__), MP_ROM_QSTR(MP_QSTR__eve) },
{ MP_ROM_QSTR(MP_QSTR__EVE), MP_OBJ_FROM_PTR(&_EVE_type) },
Copy link
Member

Choose a reason for hiding this comment

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

I'd drop the _ on the EVE class since the module name already has it. I'm ok if you'd prefer to keep it though.

Copy link
Author

Choose a reason for hiding this comment

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

I'd prefer to keep it thanks. The two modules are used like this

from _eve import _EVE
from eve import EVE

class Gameduino(_EVE, EVE):
...

So it's a little more explicit to keep the underscore naming.

Copy link
Member

Choose a reason for hiding this comment

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

Can you have EVE subclass _EVE instead? I'm a little nervous that the multi-inheritance isn't well tested.

Correct argument order
better argument naming
fix copypaste bug on C and F arguments
@tannewt
Copy link
Member

tannewt commented Feb 11, 2020

Is this ready for another look?

@jamesbowman
Copy link
Author

Yes, thanks. Comments above.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Just two comments. Getting very close! I'd love to chat about how we can use _eve for the internal display stuff too. I'd love to see a CircuitPython prompt on a TV via EVE HDMI. :-)

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Much more readable! Thank you so much!

@tannewt tannewt merged commit e97b0cf into adafruit:master Feb 13, 2020
@ladyada
Copy link
Member

ladyada commented Feb 13, 2020

nice work @jamesbowman !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants