pycom / pycom-micropython-sigfox

A fork of MicroPython with the ESP32 port customized to run on Pycom's IoT multi-network modules.
MIT License
197 stars 167 forks source link

PYCOM : make option MICROPY_FLOAT_IMPL #470

Closed rcolistete closed 3 years ago

rcolistete commented 3 years ago

Implement make option MICROPY_FLOAT_IMPL on Pycom/ESP32, so it is simpler to build Pycom/ESP32 firmware with single or double precision float point numbers. For example : [esp32]$ make -j8 MICROPY_FLOAT_IMPL=double instead of changing the 'esp32/mpconfigport.h', line : #define MICROPY_FLOAT_IMPL (MICROPY_FLOAT_IMPL_FLOAT) to #define MICROPY_FLOAT_IMPL (MICROPY_FLOAT_IMPL_DOUBLE) Batch scripts to build many Pycom/ESP32 firmware variants with combinations of boards (WIPY LOPY SIPY GPY FIPY LOPY4), single/double precision, etc, become possible.

In this way the Pycom/ESP32 building workflow becomes more compatible with MicroPython STM32 mainline workflow.

peter-pycom commented 3 years ago

Thanks for this!

Just looking at the patch it seems that we would then always be adding to CFLAG while previously we didn't add the define there...

Shouldn't the default be the same behaviour as previously? Maybe I'm missing something haven't dug deeper

rcolistete commented 3 years ago

Thanks for this!

Just looking at the patch it seems that we would then always be adding to CFLAG while previously we didn't add the define there...

Shouldn't the default be the same behaviour as previously? Maybe I'm missing something haven't dug deeper

Could you explain more what you mean ?

By the way, using CFLAGS_EXTRA like in :

[esp32]$ make -j8 CFLAGS_EXTRA="-DMICROPY_FLOAT_IMPL=MICROPY_FLOAT_IMPL_DOUBLE"

isn't possible because Pycom MicroPython doesn't define CFLAGS_EXTRA.

peter-pycom commented 3 years ago

Thanks for this! Just looking at the patch it seems that we would then always be adding to CFLAG while previously we didn't add the define there... Shouldn't the default be the same behaviour as previously? Maybe I'm missing something haven't dug deeper

Could you explain more what you mean ?

I just want to rule out regressions. So users not aware of this change don't need to do anything and will retain current behaviour.

To me it looks as if

By the way, using CFLAGS_EXTRA like in :

[esp32]$ make -j8 CFLAGS_EXTRA="-DMICROPY_FLOAT_IMPL=MICROPY_FLOAT_IMPL_DOUBLE"

isn't possible because Pycom MicroPython doesn't define CFLAGS_EXTRA.

Mhm. Maybe adding support for a CFLAGS_EXTRA would be a more generic solution and people could add any MICROPY_SOMETHING

rcolistete commented 3 years ago

Mhm. Maybe adding support for a CFLAGS_EXTRA would be a more generic solution and people could add any MICROPY_SOMETHING

See the PR#471 "User C modules : enable in Makefile and application.mk", which adds 'CFLAGS_EXTRA', etc, to 'esp32/Makefile' and 'esp32/'application.mk' .

rcolistete commented 3 years ago

I just want to rule out regressions. So users not aware of this change don't need to do anything and will retain current behaviour.

To me it looks as if

  • previously nothing was added to CFLAGS in the context of micropy float impl
  • with this patch, a define will always be added (one of _DOUBLE, _FLOAT or _NONE)

Ok. What about this code ? Only when MICROPY_FLOAT_IMPL is used with one of 3 keywords that it adds to CFLAGS :

# Configure floating point support
ifeq ($(MICROPY_FLOAT_IMPL),double)
CFLAGS += -DMICROPY_FLOAT_IMPL=MICROPY_FLOAT_IMPL_DOUBLE
else
ifeq ($(MICROPY_FLOAT_IMPL),none)
CFLAGS += -DMICROPY_FLOAT_IMPL=MICROPY_FLOAT_IMPL_NONE
else
ifeq ($(MICROPY_FLOAT_IMPL),single)
CFLAGS += -DMICROPY_FLOAT_IMPL=MICROPY_FLOAT_IMPL_FLOAT
endif
endif
endif
peter-pycom commented 3 years ago

Mhm. Maybe adding support for a CFLAGS_EXTRA would be a more generic solution and people could add any MICROPY_SOMETHING

See the PR#471 "User C modules : enable in Makefile and application.mk", which adds 'CFLAGS_EXTRA', etc, to 'esp32/Makefile' and 'esp32/'application.mk' .

So, are you saying that, yes, adding CFLAGS_EXTRA (like in #471) would be sufficient to cover the ability to choose the float impl and hence it would replace this PR #470 ?

rcolistete commented 3 years ago

Mhm. Maybe adding support for a CFLAGS_EXTRA would be a more generic solution and people could add any MICROPY_SOMETHING

See the PR#471 "User C modules : enable in Makefile and application.mk", which adds 'CFLAGS_EXTRA', etc, to 'esp32/Makefile' and 'esp32/'application.mk' .

So, are you saying that, yes, adding CFLAGS_EXTRA (like in #471) would be sufficient to cover the ability to choose the float impl and hence it would replace this PR #470 ?

No, I'm pointing to other PR which includeds CFLAGS_EXTRA. PR#471 is very important to allow user C modules in Pycom MicroPython.

After a Internet search, I don't see one example at all using this flag (CFLAGS_EXTRA=...) to compile double precision, besides the Makefile. So this flag is less known by the community, not cited in any docs/tutorial/forum, but only in git issues/etc.

Again, it is a lot more practical and known to use :

$ make MICROPY_FLOAT_IMPL=double

So, this PR#470 would be useful. For example, I'm building 48 versions of Pycom MicroPython firmwares for xxPy boards, without/with PyBytes, with single/double precision, without/with ulab module.. The script becomes simpler with ''MICROPY_FLOAT_IMPL=double" option, without the need to have a branch with 'esp32/mpconfigport.h' modified.

peter-pycom commented 3 years ago

Mhm. Maybe adding support for a CFLAGS_EXTRA would be a more generic solution and people could add any MICROPY_SOMETHING

See the PR#471 "User C modules : enable in Makefile and application.mk", which adds 'CFLAGS_EXTRA', etc, to 'esp32/Makefile' and 'esp32/'application.mk' .

So, are you saying that, yes, adding CFLAGS_EXTRA (like in #471) would be sufficient to cover the ability to choose the float impl and hence it would replace this PR #470 ?

No, I'm pointing to other PR which includeds CFLAGS_EXTRA. PR#471 is very important to allow user C modules in Pycom MicroPython.

After a Internet search, I don't see one example at all using this flag (CFLAGS_EXTRA=...) to compile double precision, besides the Makefile. So this flag is less known by the community, not cited in any docs/tutorial/forum, but only in git issues/etc.

Again, it is a lot more practical and known to use :

$ make MICROPY_FLOAT_IMPL=double

So, this PR#470 would be useful. For example, I'm building 48 versions of Pycom MicroPython firmwares for xxPy boards, without/with PyBytes, with single/double precision, without/with ulab module.. The script becomes simpler with ''MICROPY_FLOAT_IMPL=double" option, without the need to have a branch with 'esp32/mpconfigport.h' modified.

ok. fair enough.

Ok. What about this code ? Only when MICROPY_FLOAT_IMPL is used with one of 3 keywords that it adds to CFLAGS :

yes, that looks more like zero chance of regression! thanks

ifeq ($(MICROPY_FLOAT_IMPL),single)
CFLAGS += -DMICROPY_FLOAT_IMPL=MICROPY_FLOAT_IMPL_FLOAT

how about?

ifeq ($(MICROPY_FLOAT_IMPL),float)
rcolistete commented 3 years ago

how about?

ifeq ($(MICROPY_FLOAT_IMPL),float)

It departs from currently use of MICROPY_FLOAT_IMPL by MicroPython official builds. See STM32 port, for example :

So it is important to use 'single' / 'double'.

rcolistete commented 3 years ago

Ok. What about this code ? Only when MICROPY_FLOAT_IMPL is used with one of 3 keywords that it adds to CFLAGS :

# Configure floating point support
ifeq ($(MICROPY_FLOAT_IMPL),double)
CFLAGS += -DMICROPY_FLOAT_IMPL=MICROPY_FLOAT_IMPL_DOUBLE
else
ifeq ($(MICROPY_FLOAT_IMPL),none)
CFLAGS += -DMICROPY_FLOAT_IMPL=MICROPY_FLOAT_IMPL_NONE
else
ifeq ($(MICROPY_FLOAT_IMPL),single)
CFLAGS += -DMICROPY_FLOAT_IMPL=MICROPY_FLOAT_IMPL_FLOAT
endif
endif
endif

So I've commited this code.

peter-pycom commented 3 years ago

thx. created internal PR https://github.com/pycom/Firmware-Development/pull/174 we'll keep you posted

rcolistete commented 3 years ago

This PR was accepted in implemented in firmware 1.20.2.r2 announced in 2020/11/24.

peter-pycom commented 3 years ago

thanks again @rcolistete!