-
Notifications
You must be signed in to change notification settings - Fork 49
implemented memory saving changes #45
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for doing this. One question...
if self.enable_crc and self.crc_error: | ||
warn("CRC error, packet ignored") | ||
if self.enable_crc and self.crc_error(): | ||
self.crc_error_count += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jerryneedell q: with warn
removed, user will not be notified of a CRC error. Would this be handled in user-code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes -- something like this
crc_count = 0
while True:
# Look for a new packet: only accept if addresses to my_node
packet = rfm9x.receive(with_ack=True, with_header=True)
if rfm9x.crc_error_count != crc_count:
crc_count = rfm9x.crc_error_count
print("CRC error ",crc_count)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In many cases -- user may not care or want to know -- the packet will just be ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments. I think you may be onto some memory improvements more broadly.
_RH_RF95_SPREADING_FACTOR_4096CPS = const(0xC0) | ||
_RH_RF95_TX_CONTINUOUS_MOE = const(0x08) | ||
_RH_RF95_AGC_AUTO_ON = const(0x04) | ||
_RH_RF95_SYM_TIMEOUT_MSB = const(0x03) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these unused const values kept in memory? If they are used, they are usually copied in place and the name dropped. Maybe we need to modify how const works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this made any difference. I took them out since they were not used.
def tx_done(self): | ||
"""Transmit status""" | ||
return (self._read_u8(_RH_RF95_REG_12_IRQ_FLAGS) & 0x8) >> 3 | ||
|
||
def rx_done(self): | ||
"""Receive status""" | ||
return (self._read_u8(_RH_RF95_REG_12_IRQ_FLAGS) & 0x40) >> 6 | ||
|
||
def crc_error(self): | ||
"""crc status""" | ||
return (self._read_u8(_RH_RF95_REG_12_IRQ_FLAGS) & 0x20) >> 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much memory does this save? Seems like it'd very close to the cost of three instances of _RegisterBits
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main saving is that register bits creates the getter and setter -- these are all read only so they are never set. still -- it's not huge savings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getter and setter functions should be shared though because it's three instances of the same class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t understand. There is no setter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that with registerbits the unused setter does not cost anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The function objects in the data descriptor class (registerbits) should be shared amongst all instances if my mental model is correct.
Updating https://github.com/adafruit/Adafruit_CircuitPython_RFM9x to 2.0.1 from 2.0.0: > Merge pull request adafruit/Adafruit_CircuitPython_RFM9x#45 from jerryneedell/jerryn_size Updating https://github.com/adafruit/Adafruit_CircuitPython_LED_Animation to 2.3.1 from 2.3.0: > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#51 from cjsieh/customcolorchase > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#52 from kattni/sequence-fix Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 3.1.1 from 3.1.0: > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#41 from brentru/example-fix Updating https://github.com/adafruit/Adafruit_CircuitPython_NTP to 2.1.0 from 2.0.0: > Merge pull request adafruit/Adafruit_CircuitPython_NTP#11 from theelectricmayhem/patch-1
No functional changes.
Made several changes to reduce the memory usage:
Tested on STM32F405 -- before an after changes
gc.mem_free() shows an improvement of ~2Kbytes
before changes
after changes
I have also tested "freezing" this into the feather_m0_rfm9x BSP CP build and It can be made to fit with some modules removed from the standard CP build -- once this PR is accepted, I will complete that testing and submit a PR for it.
There are no breaking changes in this PR unless a user was accessing self.rx_done,self.tx_done or self.crc_error from user code. That is not done in any of the examples.