RFC: stmhal: Very early version of machine module for stmhal.

[dhylands]

I'm just posting this to see if the direction I'm going in is right.

I'm pulling truly common functions out of modpyb.c and putting them into machine.c

modmachine.c and modpyb.c will use the same functions from machine.c

[dpgeorge]

Looks good to me. But I see no reason to have a separate machine.c file, all the functions can go straight in modmachine.c (and be exported from there for pyb module).

Also, I guess we want to add bootloader to the machine module...? Not all machines have it though.

[dhylands]

I think that bootloader makes sense for the platforms that support it.

I had intended to add it to modmachine, and it looks like I just forgot - Doh.

[dhylands]

I rebased to the latest master.

[dpgeorge]

Merged in 504420c.

I left pyb.main in modpyb because it's not clear that machine is the right module for it.

[dhylands]

For making modules like Pin/UART compatible with the new API, do we want to make them compatible with both APIs? For example, in the old API there was Pin.AF_OD and we used af=, under the new API, its ALT_OD and alt= and callback= changed to irq=, etc.

So we could add support for both variants, and put the old ones under a #ifdef that we eventually turn off. Or we could duplicate the old and have pyb.Pin and machine.Pin be separate types.

[dpgeorge]

I started working on machine.Pin, just to see how easy/hard it would be; see 2f96b19.

It looks like we can make Pin compatible with both pyb and machine modules, just by providing all the constants, and multiple keyword args.

Definitely we must provide both machine and pyb classes. The firmware must be backwards compatible.

So we could add support for both variants

Yes.

and put the old ones under a #ifdef that we eventually turn off

Yes. Eg MICROPY_HW_LEGACY_PYB

Or we could duplicate the old and have pyb.Pin and machine.Pin be separate types.

We should try to make them the same when possible. If it's not possible then we can create separate types, on a case-by-case basis.

Even if we have separate types, we might still be able to re-use certain methods across the 2 types.

[dhylands]

I've always been annoyed by the use of bare ints for the indicies into the mp_arg_t and mp_arg_val_t arrays.

So I thought I'd propose a couple alternatives to consider.

The simplest would to create a enum which provide the indicies (this example would be for pin_obj_init_helper in your new version)

enum {ARG_MODE, ARG_PULL, ARG_AF, ARG_VALUE, ARG_ALT };

and then use vals[ARG_MODE].u_int instead of vals[0].u_int.

Another approach would be to create a struct for the vals. Instead of:

    mp_arg_val_t vals[PIN_INIT_NUM_ARGS];
    mp_arg_parse_all(n_args, args, kw_args, PIN_INIT_NUM_ARGS, pin_init_args, vals);

we could do it:

    struct { mp_arg_val_t mode, pull, af, value, alt; } vals;
    mp_arg_parse_all(n_args, args, kw_args, PIN_INIT_NUM_ARGS, pin_init_args, &vals.mode);

and then use vals.mode.u_int instead of vals[0].u_int
You could also use (mp_arg_val_t *)&vals instead of &vals.mode.

C99 says that the non-bitfield elements within a struct have to be allocated in the same order that they're declared, and since each and every element is the same type, the alignment should be the same as with an array.

Anyways - I just thought I'd throw those two ideas out to see if they get any traction.

[dpgeorge]

Great ideas @dhylands! Yes, it's really error prone to have the bare ints as indices, and also it's difficult to update when you insert a new arg somewhere not at the end of the list.

Both proposals are good, but somehow I prefer the struct variant, since it doesn't introduce any new identifiers into the global/local namespace, and it's a bit shorter to write.

[dhylands]

Yeah - I think I prefer the struct variant as well.

[pfalcon]

I didn't commented on this before, but now with #1555 (comment) in mind:

We should try to make them the same when possible. If it's not possible then we can create separate types, on a case-by-case basis.
Even if we have separate types, we might still be able to re-use certain methods across the 2 types.

I'd recommend against that. Please take a chance to write new clean code for new clean API. There's no need to spend effort trying to keep new and old code compatible, instead, new code should be refactored as many time as needed to be clean, general, and reusable across ports. And API of course should be finalized first (or otherwise, it's just wipy hardware API, which is as good as original pyb API), and until it is, there will certainly be changes in new code too.

[dpgeorge]

There's no need to spend effort trying to keep new and old code compatible

The thing is, there has been a lot of effort in the past 2 years making working perihperals, fixing bugs and corner cases based on real-word use, and we don't want to throw that away and start again.

It would be nice to make it clean, and we should try, but it needs to be a gradual evolution.

and reusable across ports

Yes, reusing method tables and boiler-plate code would be nice, but there's stuff like DMA support which is deeply ingrained in the code and going to be hard to abstract properly across ports.

And API of course should be finalized first

I believe a large subset of the API is finalised (eg basic UART, I2C and SPI creation and methods), and we should work on implementing that first. The API will grow to include more features, but that's going to take a long time to come to an agreement on the design.

[pfalcon]

The thing is, there has been a lot of effort in the past 2 years making working perihperals, fixing bugs and corner cases based on real-word use, and we don't want to throw that away and start again.

Of course! Hopefully we want to copy that code to new file and let both copies live separate lives. As lot of effort over past 2 years were done to pyb, hopefully it won't change a lot from now on (of course, issues like #1533 should be fixed). And new "machine" code can and should change, maybe even rewritten from scratch for some methods. And that should be done freely, without additional constraint that the same underlying function should be reusable "pyb" and "machine". Just my 2 cents.

Yes, reusing method tables and boiler-plate code would be nice,

Yeah, so would be nice to have basic/reference implementation which "just works" (though maybe not for all cases). Like, bitbanging. Pity nobody has arrived to that, I'd put it in my queue - once SPI/I2C master API is actually finalized.

[dpgeorge]

Hopefully we want to copy that code to new file and let both copies live separate lives.

Well, we could do that. But then bug fixes would need to go to both copies. And how would we name the new one? Would we follow cc3200 and create a mods/ directory? Or name them, eg, machinepin.c, machinei2c.c?

without additional constraint that the same underlying function should be reusable "pyb" and "machine"

I want to provide pyb module for some time to facilitate conversion from this API to the new one, and would be good to do this without using heaps of ROM. I don't know exactly how much extra ROM it would take having total duplicates of pyb and machine, maybe it's not much; 20k would be acceptable.

[pfalcon]

And how would we name the new one?

Don't know for sure.

Would we follow cc3200 and create a mods/ directory?

IMHO this complicates working with codebase.

Or name them, eg, machinepin.c, machinei2c.c?

Doing it this way could facilitate code reuse, and I like well-separated stuff. But then there may be too many of such files...

So, first step could be just like @dhylands did: create more or less exact copy of modpyb, exccpt that I, well, argue for putting back code removed from there.

good to do this without using heaps of ROM.

But you always say there're tons of ROM! ;-) That's a good way to put it to use.

[dpgeorge]

We could use machpin.c, machi2c.c etc to keep it short.

If we want to make it generic and reusable by other ports, maybe it should go in extmod/machine/ from the very beginning? And then simply be pin.c, i2c.c, etc. (Would still need implementation specific stuff in stmhal/ though.)

So, first step could be just like @dhylands did: create more or less exact copy of modpyb, exccpt that I, well, argue for putting back code removed from there.

But if we copy pin.c, what is it copied to? I'd say lets not put stuff back in modpyb.c yet, just do it on a function-by-function basis when they need to be changed from the original implementation.

But you always say there're tons of ROM! ;-)

Hopefully pyblite will be out soon (factory has started manufacturing this week...) and that only has 512k ROM, and I would like to provide pyb module on that board.

[pfalcon]

We could use machpin.c, machi2c.c etc to keep it short.

My concern is not length of names, but having 20 such files for example.

If we want to make it generic and reusable by other ports, maybe it should go in extmod/machine/ from the very beginning? And then simply be pin.c, i2c.c, etc. (Would still need implementation specific stuff in stmhal/ though.)

I don't spread my thoughts that so far. Currently of new stuff I'm for example thinking of: 1) how to reuse sleep_ms/us/etc. Python functions implementations. Has MPHAL refactoring on critical path. General idea is to do like py/stream.c . Probably put to extmod/ - but those won't be complete modules, just methods to put per-port modules (maybe it should be other way around - have complete module where ports can contribute extra functions, but I don't want to stray that far.) 2) How to let ports' I2C/SPI classes reuse common bitbanging impl. Actually finalizing I2C/SPI classes is on critical path, and for impl, bunch of other things are. I'd propose having id=-1 to HW API constructors meaning "use emulated" that can be bitbanging implementation, or virtual OS timer, etc.

But if we copy pin.c, what is it copied to? I'd say lets not put stuff back in modpyb.c yet, just do it on a function-by-function basis when they need to be changed from the original implementation.

Sounds good, again, I wanted to share the message that trying to shape new HW API modules in terms of existing pyb functions and maintaining compatibility on that level going forward is not ideal use of resources and limits freedom to shape better implementation.

[dhylands]

So, first step could be just like @dhylands did: create more or less exact copy of modpyb, exccpt that I, well, argue for putting back code removed from there.

Actually I copied cc3200's modmachine.c, dictionary definition, and then moved functions from stmhal's modpyb.c and uncommented bits as they were implemented. My intention was then to go back through modpyb.c and figure out what stuff it had that modmachine.c didn't and figure out where those bits should go.