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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

bricks/movehub: Use ERROR_REPORTING_NONE. #240

Merged
merged 2 commits into from
Mar 4, 2024
Merged

Conversation

laurensvalk
Copy link
Member

@laurensvalk laurensvalk commented Mar 1, 2024

This saves about 2.6KB without making the errors any less terse than they already were.

馃く

For example

from pybricks.hubs import MoveHub2

will still give the slightly more unhelpful error:

Traceback (most recent call last):
  File "movespace.py", in <module>
ImportError: 

So no great loss. I dug into this when adding a call to raise_msg, which added a bit too much build size for something that isn't supposed to do much.

This saves about 2.6KB without making the errors any less terse than they already were.
@coveralls
Copy link

coveralls commented Mar 1, 2024

Coverage Status

coverage: 55.495%. remained the same
when pulling 4c14de3 on movehub-errors
into a844b32 on master.

@dlech
Copy link
Member

dlech commented Mar 1, 2024

If the error output is the same, what is actually being removed that saves so much size?

@laurensvalk
Copy link
Member Author

laurensvalk commented Mar 1, 2024

Presumeably code with message handling now skipping that, but I'll definitely dig deeper before merging.

#if MICROPY_ERROR_REPORTING == MICROPY_ERROR_REPORTING_NONE
#define mp_obj_new_exception_msg(exc_type, msg) mp_obj_new_exception(exc_type)
#define mp_obj_new_exception_msg_varg(exc_type, ...) mp_obj_new_exception(exc_type)

This makes it consistent with the build option.

The previous commit saved a large amount of space, despite accidentally bringing back the verbose Pybricks error messages.
@laurensvalk
Copy link
Member Author

But I wasn't quite right about the savings, since the some of our own modules check for MICROPY_ERROR_REPORTING_TERSE but not for the stricter NONE option just enabled. So it was accidentally bringing back the verbose Pybricks error tips.

Now it saves about 4KB.

@laurensvalk
Copy link
Member Author

laurensvalk commented Mar 2, 2024

@BertLindeman , @JorgePe, would you be interested to try out this firmware to see if anything major is missing or broken?

If we really saved 4KB, we should be able to keep your move hubs going for quite a long time 馃槃

The download links are shown in the posts above.

@JorgePe
Copy link
Contributor

JorgePe commented Mar 2, 2024

@BertLindeman , @JorgePe, would you be interested to try out this firmware to see if anything major is missing or broken?

If we really saved 4KB, we should be able to keep your move hubs going for quite a long time 馃槃

The download links are shown in the posts above.

--- forget it, i read the whole text --- I sure can - I was just flashing another Move Hub for my snake. Is it available with Beta IDE or do I need to download it?

@laurensvalk
Copy link
Member Author

Thanks! I was hoping the weekend was just the right time 馃槃

Please use the download link above.

In Pybricks, begin the firmware update as usual, but instead of selecting the move hub, choose Advanced at the bottom and select the zip you just downloaded.

It doesn't matter if you do this in Pybricks beta or stable.

Things to look out for would be unexpected errors, or maybe missing errors. Or any form of unexpected behaviors, such as things continuing when there should have been an exception.

@BertLindeman
Copy link
Contributor

@BertLindeman , @JorgePe, would you be interested to try out this firmware to see if anything major is missing or broken?

If we really saved 4KB, we should be able to keep your move hubs going for quite a long time 馃槃

The download links are shown in the posts above.

will do, Laurens

@JorgePe
Copy link
Contributor

JorgePe commented Mar 2, 2024

Looks good to me. Tested Broadcasting (just observe), internal LED color and the 2 internal motors in multitask.

@BertLindeman
Copy link
Contributor

OK by me too, tested broadcast, observe, various import, name and other errors.
Made a testprogram with 20 functions so 200 subfunctions to test the program size, like this

def main_function0():
    def sub_function1(): 
        data1 = list(range(100))
    def sub_function2(): 
        data1 = list(range(100))
    def sub_function3(): 
        data1 = list(range(100))
    def sub_function4(): 
        data1 = list(range(100))
    def sub_function5(): 
        data1 = list(range(100))
    def sub_function6(): 
        data1 = list(range(100))
    def sub_function7(): 
        data1 = list(range(100))
    def sub_function8(): 
        data1 = list(range(100))
    def sub_function9(): 
        data1 = list(range(100))
    def sub_function10(): 
        data1 = list(range(100))

The compiler reports:

This program is too big. Compiled size is 6,160 bytes but the hub can only fit 3,956 bytes.

Try removing unused code or making names and strings smaller.

@laurensvalk
Copy link
Member Author

There is some loss in verbosity.

raise OSError("hi")
Traceback (most recent call last):
  File "before.py", in <module>
OSError: hi
ImportError: 
Traceback (most recent call last):
  File "after.py", in <module>
OSError: hi

1/0
Traceback (most recent call last):
  File "before.py", in <module>
TypeError: unsupported type for operator
TypeError: 
Traceback (most recent call last):
  File "after.py", in <module>
TypeError: 

from pybricks import nothing
Traceback (most recent call last):
  File "before.py", in <module>
ImportError: can't import name nothing
Traceback (most recent call last):
  File "after.py", in <module>
ImportError:

@laurensvalk
Copy link
Member Author

laurensvalk commented Mar 3, 2024

But I don't think the TERSE errors were especially helpful to begin with (already indecipherable for newcomers), so the NONE errors might be just fine, given the savings.

@laurensvalk
Copy link
Member Author

OK by me too, tested broadcast, observe, various import, name and other errors.

Thanks @BertLindeman !

@laurensvalk
Copy link
Member Author

Looks good to me. Tested Broadcasting (just observe), internal LED color and the 2 internal motors in multitask.

Awesome, thank you!

@laurensvalk laurensvalk merged commit 4c14de3 into master Mar 4, 2024
38 checks passed
@dlech dlech deleted the movehub-errors branch March 4, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants