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

QTPy Memory Error #11

Open
smithj33 opened this issue Jul 30, 2022 · 15 comments
Open

QTPy Memory Error #11

smithj33 opened this issue Jul 30, 2022 · 15 comments
Assignees

Comments

@smithj33
Copy link

Issue:
With CircuitPython 7.3.2 and the latest libraries, the TMP117 cannot be used with the QTPy due to a memory allocation error.

Resolution:
Roll back to CircuitPython 6.3.0 and the TMP117 version 6 library.

I know the QTPy has limited memory, but I would expect this would still work with CP7.

Thanks

@ladyada
Copy link
Member

ladyada commented Aug 1, 2022

CPy tends to use more RAM over time... really best option is to update your QT Py to an RP2040 since it has 10x the RAM!

@smithj33
Copy link
Author

smithj33 commented Aug 1, 2022

Thanks. I just swapped the TP117 out for an SHT40 and it works with CPy7 and the QTPy. Appears the TP117 is just a more complex sensor as the library is almost twice as large with a lot of functionality built in.

@ladyada
Copy link
Member

ladyada commented Aug 1, 2022

awesome, thank you
@evaherrada please add a note for this library that it may not work on low-ram boards such as SAMD21's

@tannewt
Copy link
Member

tannewt commented Aug 1, 2022

We could also try and optimize this library. I suspect its RAM used could be reduced by trimming functionality or simplifying APIs.

@ladyada
Copy link
Member

ladyada commented Aug 1, 2022

we could but thats unlikely to happen soon, better to warn folks until that magical day arrives

@dhalbert
Copy link
Contributor

dhalbert commented Aug 1, 2022

Suggestions for making more compact:

  1. _*_MODE values can be const(). They are just binary integers.
  2. _convert_to_integer is used only to convert the serial number into a long int. Remove this and just return the serial number as a bytearray. Using a longint causes issues on SAMD21 builds anyway.
  3. .temperature_offset, .high_limit, and .low_limit all have virtually identical code in their setters: range check of -256 to 255, and same scaled value calculation. Refactor this. Also these exceptions should really be ValueError, not AttributeError.
  4. (EDIT: ADDED) AverageCount.AVERAGE_<n>X* uses the word "average" redundantly. Shorter strings would be AverageCount.X_<n>. Same for MeasurementDelay.DELAY_*.

@dhalbert
Copy link
Contributor

dhalbert commented Aug 1, 2022

Also, class CV is duplicated in a bunch of libraries, so it will not be shared when multiple libraries are loaded. This could be factored out into its own library or added to adafruit_register.

@ladyada
Copy link
Member

ladyada commented Aug 1, 2022

@Neradoc do you want to try improving the size of this library?

@jposada202020
Copy link
Contributor

Hello there. I could work on this if needed. @Neradoc, let me know if that is ok with you!. thanks :)

@Neradoc
Copy link

Neradoc commented Jan 24, 2023

Yes (I don't actually know why I was assigned to this).

@jposada202020
Copy link
Contributor

Will do thanks :)

@jposada202020
Copy link
Contributor

I include suggestions in #13. However, we still have te memory allocation problem. Path from here will be either:

  1. Make a basic and advanced library as we did for the BME280 sensor https://github.com/adafruit/Adafruit_CircuitPython_BME280

or

  1. Remove CV and make values instead the overhead of the class to create them as we do for most of the libraries. see https://github.com/adafruit/Adafruit_CircuitPython_LIS3DH.

Either way, it would be a breaking change. Just a note, I have not done the test removing the CV class, so not sure of the result and we could end up doing solution 1.

So let me know with path do you prefer to go. Thanks @tannewt @dhalbert

@dhalbert
Copy link
Contributor

I agree with you about removing the CV stuff, which adds overhead and uses space. I would start with that first, and then we will need to update the examples and any Learn Guides.

@jposada202020
Copy link
Contributor

Will do thanks :)

@jposada202020
Copy link
Contributor

Just an update, removing the CV did not solve the problem. as you could see in 54887af. Thanks

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 a pull request may close this issue.

7 participants