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

Updates needed to build latest code on CircuitPython #122

Merged
merged 3 commits into from
Jun 1, 2020

Conversation

jepler
Copy link
Collaborator

@jepler jepler commented Jun 1, 2020

This fixes the following build error when updating ulab in CircuitPython:

../../extmod/ulab/code/vectorise.c: In function 'vectorise_vectorized_function_call':
../../extmod/ulab/code/vectorise.c:242:15: error: implicit declaration of function 'mp_obj_is_int'; did you mean 'mp_obj_list_init'? [-Werror=implicit-function-declaration]
  242 |     } else if(mp_obj_is_int(args[0]) || mp_obj_is_float(args[0])) {
      |               ^~~~~~~~~~~~~
      |               mp_obj_list_init
../../extmod/ulab/code/vectorise.c:242:15: error: nested extern declaration of 'mp_obj_is_int' [-Werror=nested-externs]
cc1: all warnings being treated as errors

@v923z
Copy link
Owner

v923z commented Jun 1, 2020

Won't we run into circular definitions? This is from obj.h:
https://github.com/micropython/micropython/blob/22806ed5df27c10131af0cedb2f7e8b134fe6e7a/py/obj.h#L1005

I have to admit, I am never sure, whether one should use lower case, or upper case with these type checks. If you know what to do, I am not against making the whole thing uniform, so that we wouldn't have to distinguish between circuitpython, and micropython.

One might have to consider, though, that macros are in-lined, so we would probably add to the size of the firmware

This fixes the diagnostic when building circuitpython:
```
../../extmod/ulab/code/approx.c:25:12: error: no previous prototype for ‘approx_python_call’ [-Werror=missing-prototypes]
 mp_float_t approx_python_call(const mp_obj_type_t *type, mp_obj_t fun, mp_float_t x, mp_obj_t *fargs, uint8_t nparams) {
            ^~~~~~~~~~~~~~~~~~
```
As far as I can tell, these are not checked for the sake of efficiency.

This silences compiler diagnostics when building CircuitPython
```
../../extmod/ulab/code/approx.c:25:12: error: no previous prototype for ‘approx_python_call’ [-Werror=missing-prototypes]
 mp_float_t approx_python_call(const mp_obj_type_t *type, mp_obj_t fun, mp_float_t x, mp_obj_t *fargs, uint8_t nparams) {
            ^~~~~~~~~~~~~~~~~~
```
@jepler
Copy link
Collaborator Author

jepler commented Jun 1, 2020

If CircuitPython ever updates from MicroPython, we'll have to change it. But for now, this seems like the easiest way.

I've updated this PR with additional fixes for building on CircuitPython, due to our compiler flags.

@v923z
Copy link
Owner

v923z commented Jun 1, 2020

If CircuitPython ever updates from MicroPython, we'll have to change it. But for now, this seems like the easiest way.

I've updated this PR with additional fixes for building on CircuitPython, due to our compiler flags.

OK. Go ahead!

@jepler jepler changed the title ndarray: Let mp_obj_is_int work on circuitpython Updates needed to build latest code on CircuitPython Jun 1, 2020
@@ -28,6 +28,7 @@

#if CIRCUITPY
#define mp_obj_is_bool(o) (MP_OBJ_IS_TYPE((o), &mp_type_bool))
#define mp_obj_is_int(x) (MP_OBJ_IS_INT((x)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, this is the easiest way. MicroPython was just trying to canonicalize their macro names.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't mean for this to be a review, just a comment.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, but a comment by the oracle qualifies as a review. Seriously, Dan, I really appreciate that you chipped in.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am only an oracle because I subscribe to all the micropython git notifcations 😄

@jepler jepler merged commit 0394801 into v923z:master Jun 1, 2020
jepler added a commit to jepler/circuitpython-ulab that referenced this pull request Jun 1, 2020
I noticed on v923z#122 that no actions were run.  I believe this is because
the paths directives had no wildcards, contrary to the examples at
https://help.github.com/en/actions/reference/workflow-syntax-for-github-actions#onpushpull_requestpaths

Eliminating the paths: restrictions entirely may be an equally valid
course of action.
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