Skip to content

implement a few size reduction changes #33

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

Merged
merged 7 commits into from
Sep 9, 2020
Merged

implement a few size reduction changes #33

merged 7 commits into from
Sep 9, 2020

Conversation

jerryneedell
Copy link
Contributor

changes to reduce the code size.
disabled pylint check for "too-many"statements" for the init() function.
without this I had to create a function to do some of the initializations. THis added 80 bytes to the code size.
The init() function is only a few statements over the 50 statement limit in pylint so it seemed better to override.

The savings for these changes is only about 140 bytes total but seems like it is worth doing.

I removed a few register initializations that were setting the registers to their default values.

Tested on RaspberryPi 4 with rfm69 bonnet and feather_m4_express with rfm69 featherwing.

@jerryneedell jerryneedell requested review from brentru and a team August 21, 2020 18:51
Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

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

@jerryneedell asking some questions, -140 bytes of space saving is 💯

@@ -300,8 +297,26 @@ def __init__(
self.preamble_length = preamble_length # Set the preamble length.
self.frequency_mhz = frequency # Set frequency.
self.encryption_key = encryption_key # Set encryption key.
# set radio configuration parameters
self._configure_radio()
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason you removed configure_radio?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

putting the code inline rather than as a function call saved 80 bytes!

# by default. Users with advanced knowledge can manually reconfigure
# for any other mode (consulting the datasheet is absolutely
# necessary!).
self.data_mode = 0b00 # Packet mode
Copy link
Member

Choose a reason for hiding this comment

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

I removed a few register initializations that were setting the registers to their default values.

This got me thinking - are any of these values default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

data_mode and modulation_type are the defaults -- I'll remove them
saved 12 bytes!

self.send(
data,
b"!",
Copy link
Member

Choose a reason for hiding this comment

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

Why is b"!", being sent instead of the data?

Copy link
Contributor Author

@jerryneedell jerryneedell Aug 21, 2020

Choose a reason for hiding this comment

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

just seemed simpler to do it this way. The ACK packet is always just "!"

Copy link
Member

Choose a reason for hiding this comment

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

Note that the original bytes("!", "UTF-8") stores two strings plus the byte codes to convert them to a bytes object. data = b"!"will save those two strings and only cost the assign byte code assuming the namedata` is used elsewhere.

@jerryneedell
Copy link
Contributor Author

jerryneedell commented Aug 22, 2020

@brentru hold off on merging this....
There appears to be a very subtle issue that, so far, impacts m0 builds. I'm not completely convinced it is new, but seems to be worse with this PR in place. IT may be some sort of timing issue.

I will need to do a lot more testing to resolve this.

The issue only occurs when I have an M0 receiving (does not seem to matter which kind) and I am sending "reliable datagrams" when the internal sequence counter is between 128 and 255 (0x80-0xff) the receiver frequently is missing the MSB of the internal sequence count (identifier) ... This results in an ACK failure. I have no idea what is going wrong yet. I have not seen the issue on an RPi or an M4. For these test, I have been using an RPi as one side of the test.

I'll keep working on it ....

edited to add -- it is not a new problem ... it appears to occur in a similar but even more subtle way with the current release.

@brentru
Copy link
Member

brentru commented Aug 25, 2020

Ok - holding off until that issue is cleared.

@jerryneedell
Copy link
Contributor Author

Still investigating the SPI baud rate issue, but wanted to get some of these updated in place. Refactored some code to eliminate a function that was used for writing to the FIFO. now done via the general write_from().

@jerryneedell
Copy link
Contributor Author

@brentru after the discussion at yesterdays CP weekly, I think this is ready to go. There are some long term changes that may be worth looking into but I don't think we have to wait. Let me know if you have any questions about this or if you want any additional changes.

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

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

LGTM!

@jerryneedell jerryneedell merged commit c0b9bdf into adafruit:master Sep 9, 2020
@jerryneedell jerryneedell deleted the jerryn_size branch September 9, 2020 17:39
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Sep 10, 2020
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