Skip to content

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

Merged
merged 6 commits into from
Jun 18, 2020
Merged

implemented memory saving changes #45

merged 6 commits into from
Jun 18, 2020

Conversation

jerryneedell
Copy link
Contributor

No functional changes.

Made several changes to reduce the memory usage:

  1. Removed use of "warnings" for crc errors - replaced with a counter
  2. Replaces some instances of "registerbits" with simple function calls ( rx_done,tx_done,crc_error are now functions and are only ever read so the setter/getter was not necessary)
  3. made ACK packet a fixed byte string -- no need to call bytes to create it
  4. remove unused constants

Tested on STM32F405 -- before an after changes
gc.mem_free() shows an improvement of ~2Kbytes

before changes

Press any key to enter the REPL. Use CTRL-D to reload.
Adafruit CircuitPython 5.4.0-beta.1-13-g9285252fb on 2020-06-12; Feather STM32F405 Express with STM32F405RG
>>> import gc
>>> gc.mem_free()
91088
>>> import adafruit_rfm9x
>>> gc.mem_free()
76112
>>> gc.collect()
>>> gc.mem_free()
76080
>>> 

after changes

Press any key to enter the REPL. Use CTRL-D to reload.
Adafruit CircuitPython 5.4.0-beta.1-13-g9285252fb on 2020-06-12; Feather STM32F405 Express with STM32F405RG
>>>
>>> import gc
>>> gc.mem_free()
91088
>>> import adafruit_rfm9x
>>> gc.mem_free()
79344
>>> gc.collect()
>>> gc.mem_free()
79312
>>> 

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.

@jerryneedell jerryneedell requested a review from brentru June 18, 2020 18:00
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.

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
Copy link
Member

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?

Copy link
Contributor Author

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)

Copy link
Contributor Author

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

@brentru brentru merged commit 0d355c0 into adafruit:master Jun 18, 2020
@jerryneedell jerryneedell deleted the jerryn_size branch June 18, 2020 18:28
Copy link
Member

@tannewt tannewt left a 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)
Copy link
Member

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.

Copy link
Contributor Author

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.

Comment on lines +591 to +601
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
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@jerryneedell jerryneedell Jun 18, 2020

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?

Copy link
Member

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.

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jun 23, 2020
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
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