Move i2c_struct.UnaryStruct's byte array to init - #62
Conversation
tannewt
left a comment
tannewt
left a comment
There was a problem hiding this comment.
Could we allocate it once on first use? It'll cost us a little code size but save instance size for unused accessors.
|
@tannewt Are we worried at all about concurrency / two instances writing on the shared buffer? Edit: I have implemented a class wide version of all the i2c_ libraries (except at the moment i2c_struct_array). See subsequent comments. Here is the code change for UnaryStruct I'm currently testing. Similar for the others. class UnaryStruct:
_scratch = bytearray(1) # ONE buffer shared by every UnaryStruct instance
def __init__(self, register_address, struct_format):
self.format = struct_format
self.address = register_address
self.size = struct.calcsize(struct_format)
need = 1 + self.size
if len(UnaryStruct._scratch) < need: # grow at class-def time to fit the biggest
UnaryStruct._scratch = bytearray(need)
def __get__(self, obj, objtype=None):
buf = UnaryStruct._scratch
buf[0] = self.address # MUST set every call now (shared, gets clobbered)
with obj.i2c_device as i2c:
i2c.write_then_readinto(buf, buf, out_end=1, in_start=1, in_end=1 + self.size)
return struct.unpack_from(self.format, buf, 1)[0]
def __set__(self, obj, value):
buf = UnaryStruct._scratch
buf[0] = self.address
struct.pack_into(self.format, buf, 1, value)
with obj.i2c_device as i2c:
i2c.write(buf, end=1 + self.size) |
|
@tannewt Here are the footprint changes measured via gc.collect() + gc.mem_free() paired before and after. Sadly this particular optimization doesn't help TMP117 a ton because there are only 9 Bit(s) accessors and 5 UnaryStruct accessors -- on the order of 300 total bytes saved. The CV attribute naming infrastructure in TMP117 takes 1280 bytes which dwarfs in savings. Per instance space footprint
Given the above, do you think I should fold in the class wide buffer version of these? I haven't yet considered the register_ versions that I know @FoamyGuy has most recently worked on. If a similar savings were available there it could be about a 1K savings for the BMP5xx driver's 29 accessors. |
Not currently. We don't support concurrency from Python. Even with asyncio property reads are sync. I hadn't thought about a class-wide buffer and now I'm wondering if a module-wide one would be even better. What do you think? |
|
In my mind I had thought RO versions would inherit the same storage but now
that I think of that it makes sense they don’t. I can try some
instrumentation after I brush up on the module wide syntax.
…On Wed, Aug 5, 2026 at 9:43 AM Scott Shawcroft ***@***.***> wrote:
*tannewt* left a comment (adafruit/Adafruit_CircuitPython_Register#62)
<#62 (comment)>
@tannewt <https://github.com/tannewt> Are we worried at all about
concurrency / two instances writing on the shared buffer?
Not currently. We don't support concurrency from Python. Even with asyncio
property reads are sync.
I hadn't thought about a class-wide buffer and now I'm wondering if a
module-wide one would be even better. What do you think?
—
Reply to this email directly, view it on GitHub
<#62?email_source=notifications&email_token=ABEWDBLAIBA6TTMJGCJFDYT5INP45A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMJZGQ3DENJZGI3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5194625926>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABEWDBP3G2IH4CPKRFJDOWD5INP45AVCNFSNUABEKJSXA33TNF2G64TZHM3TKMBQGYYDCNJ3JFZXG5LFHM2TANJWGU2DQOBZGKQXMAQ>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/ABEWDBJU74W4AWXXLIMEADL5INP45A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMJZGQ3DENJZGI3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/ABEWDBMN6LD2SZCURCX7J6D5INP45A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMJZGQ3DENJZGI3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
@tannewt I've done some research and i don't think there is any big benefit to a module wide one for most of i2c_ modules. I've verified that ROBit & Bits actually use the parent class's storage when they inherit RWBit & Bits definition. So I don't think there is any win at all there. The Struct and Struct array might benefit slightly but I'm not sure it is worth it. Already ROUnary inherits Unary and all Classes start out at one byte across all instances until instantiated and grown dynamically. I'm also not sure that allocating on first use wins us much. If there are a dozen accessors in a library, what is the chance that none of them are ever used and the first use of ROBits for example amortizes all RO/WBits instances at once. I tested new versions of adafruit_register.register_Bit(s) and Struct at the class level and here are the savings:
The memory footprint savings for adafruit_bmp5xx (29 accessors) with this change is: 3728 - 2800 = 928 so almost 1K I'd really like to know if I should push these class-wide variables changes up or not. My original commit was just to bring the i2c_struct.UnaryStruct into line with all the others using an instance level buffer rather than allocating it on each get/set -- I could go either way on the class wide one. I'm ready to push all the libraries and have tested all but i2c_struct.Struct and i2c_struct_array because I don't have hardware using those. HW using i2c_struct_array: PCA9685 Or if you want me to change all of them to module wide, I think I know how to do the change.. |
Yup, give module wide a try. That will leave us with one buffer to share. Note that CP/MP have a minimum allocation size so a one byte allocation is really 16 bytes x 2. One for the buffer and one for the wrapper object. |
Two ways to go with the module wide. New class to store the module wide shared variable: e.g. new class named _scratch with shared buffer named buf. Called out in each other class as '_scratch.buf' Global variable called eg as 'buf' but needs a noqa PLW0603 -- simpler to write throughout if you're ok with disabling the RUFF complaint. |
I think if you capitalize it right it won't complain. I'd do this. The new class idea is unusual. |
2 participants
After discussing with @tannewt he agreed this change was a reasonable one to make.
This brings i2c_struct.UnaryStruct's bytearray allocation into the class init so it matches all the other classes in the adafruit_register group. Because the UnaryStruct is used by things like tmp117._raw_temperature it was creating and releasing bytestrings each call. Now one is permanently allocated -- again like all the other classes here.
My testing shows that for the tmp117 library this new way of pre-allocating the bytearray for each of the UnaryStruct register accessors increases the per instance bytecount from 42 bytes to 74 bytes so a net increase per accessor of 32 extra bytes. However we avoid garbage collection churn.
I have been running this new library on 2 TMP119s and 1 TMP117 and 4 MAX1704x for a number of hours with no issues. I also read, wrote and read back the changed value of device registers on the TMP119 and MAX1704x devices.
All that being said, I hope given the very wide use of the adafruit_register libraries that whomever reviews is satisfied that my change is a reasonable and sane one.