v923z / micropython-ulab

a numpy-like fast vector module for micropython, circuitpython, and their derivatives
https://micropython-ulab.readthedocs.io/en/latest
MIT License
430 stars 117 forks source link

Fix build for MICROPY_OBJ_REPR_D configurations #531

Closed dpgeorge closed 2 years ago

dpgeorge commented 2 years ago

ulab currently does not compile when MICROPY_OBJ_REPR_D is selected as the object representation. This representation is useful when you don't want float objects to use heap memory (they are allocated in-place, like small ints).

This PR fixes ulab so it does build with this option. The main items fixed are:

@v923z is this something you're interested in having? In my opinion it's a good improvement and should not change in anyway the existing non-obj-repr-D builds.

v923z commented 2 years ago

@dpgeorge Many thanks for taking a stab at it, and especially, fixing the inconsistencies in the use of the ROM constants! At the moment the CI fails, but perhaps it wasn't meant to succeed at this stage. I have looked at the error message, but it didn't tell me too much.

@v923z is this something you're interested in having? In my opinion it's a good improvement and should not change in anyway the existing non-obj-repr-D builds.

I definitely agree with you, and people should be able to compile the firmware with the D representation. It just never occurred to me. I would just like to ask you to increment the version number in https://github.com/v923z/micropython-ulab/blob/00139bdd9684bd9d10b0d89fa8fbb0906f2d05af/code/ulab.c#L36, so that we can keep track of the changes.

dpgeorge commented 2 years ago

I definitely agree with you, and people should be able to compile the firmware with the D representation.

Great!

At the moment the CI fails, but perhaps it wasn't meant to succeed at this stage.

I have tried to fix this by apt-get install gcc-multilib.

I would just like to ask you to increment the version number in

Now done, bumped to 5.0.8.

v923z commented 2 years ago

At the moment the CI fails, but perhaps it wasn't meant to succeed at this stage.

I have tried to fix this by apt-get install gcc-multilib.

It seems to be OK now. (The macintosh builds haven't yet finished.)

This is still a draft. Is there something missing?

v923z commented 2 years ago

Well, my comment was actually premature, because the firmware could be compiled, but not linked on the mac.

dpgeorge commented 2 years ago

The macOS build logs say:

ld: warning: The i386 architecture is deprecated for macOS (remove from the Xcode build setting: ARCHS)

So I think just don't build the nanbox version on macOS.

v923z commented 2 years ago

The macOS build logs say:

ld: warning: The i386 architecture is deprecated for macOS (remove from the Xcode build setting: ARCHS)

So I think just don't build the nanbox version on macOS.

OK. Do you want to take that out of the matrix in the workflow file?

dpgeorge commented 2 years ago

I changed it in build.sh so it only builds the nanbox variant on linux. Let's see how it goes.

v923z commented 2 years ago

I changed it in build.sh so it only builds the nanbox variant on linux. Let's see how it goes.

This passes now, so just let me know, whenever you are ready to merge this.

dpgeorge commented 2 years ago

I have updated the change to how constants are defined and used to have more comments. Also simplified the constants in code/numpy/numpy.c to make use of these new helper macros (ULAB_DEFINE_FLOAT_CONST and ULAB_REFERENCE_FLOAT_CONST).

These changes have made it so ulab no longer builds with object representation C (used on esp8266, and one stm32 board). Prior to this PR ulab would build with repr C but some parts would be broken (eg default float values for arguments).

I can also try to get repr C working in this PR, would be good.

dpgeorge commented 2 years ago

Object representation C is now working (tested with a local version of unix).

This is now ready for final review.

v923z commented 2 years ago

@dpgeorge Fantastic, many thanks for sticking with it till the end, and for contributing to ulab!

dpgeorge commented 2 years ago

My pleasure! Thanks for the quick turn around and merging!