Skip to content

Chunk buffer sends into 64 byte chunks #29

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 11, 2019
Merged

Chunk buffer sends into 64 byte chunks #29

merged 2 commits into from
Jul 11, 2019

Conversation

martymcguire
Copy link
Contributor

Prevents errors with unsent data on large header values, long URL paths

Addresses the specific issue from #28

@martymcguire
Chunk buffer sends into 64 byte chunks
8e65459 
Prevents errors with unsent data on large header values, long URL paths
@martymcguire martymcguire requested a review from ladyada March 13, 2019 19:47
@martymcguire
Copy link
Contributor Author

update! gonna rewrite this to use MemoryView instead of slices, to avoid all the newly allocated arrays.

@martymcguire martymcguire changed the title Chunk buffer sends into 64 byte chunks [WIP] Chunk buffer sends into 64 byte chunks Mar 13, 2019
@martymcguire
Copy link
Contributor Author

martymcguire commented Mar 13, 2019 •
edited
Loading

thanks to advice from @ladyada i was able to confirm some memory savings over my first attempt by using memoryview instead of buffer slicing and dropping the temp vars.

all numbers come from adding a debug statement into the for loop in socket_write, and are only a benchmark of my specific demo.

no memoryview:

Free mem:  145104
Free mem:  143616
Free mem:  142128
Free mem:  140640
Free mem:  139152
Free mem:  137664
Free mem:  136192
Free mem:  147440
Free mem:  147904
Free mem:  147936

memoryview:

Free mem:  145104
Free mem:  143680
Free mem:  142256
Free mem:  140832
Free mem:  139408
Free mem:  137984
Free mem:  136560
Free mem:  147392
Free mem:  147872
Free mem:  147872

memoryview + no chunksize var:

Free mem:  145120
Free mem:  143696
Free mem:  142272
Free mem:  140848
Free mem:  139424
Free mem:  138000
Free mem:  136576
Free mem:  147408
Free mem:  147888
Free mem:  147888

memoryview + no chunksize var + no chunk_buffer var:

Free mem:  145216
Free mem:  143792
Free mem:  142368
Free mem:  140944
Free mem:  139520
Free mem:  138096
Free mem:  136672
Free mem:  147504
Free mem:  147984
Free mem:  147984

@martymcguire martymcguire changed the title [WIP] Chunk buffer sends into 64 byte chunks Chunk buffer sends into 64 byte chunks Mar 13, 2019
@jerryneedell
Copy link
Contributor

oops - somehow I missed this request for review. I won't be able to do any testing until next week, sorry. If others have tested it and are comfortable with the change, I have no concern with merging it.

@maholli
Copy link

maholli commented Jul 5, 2019

Chiming in with a success story.

Using this PR I was able to successfully send large base64 images to adafruit.io that were previously unsuccessful. Much easier than breaking it up into multiple io.send_data calls. 👍 @jerryneedell

@maholli
Copy link

maholli commented Jul 5, 2019

Chiming in with a success story.

Using this PR I was able to successfully send large base64 images to adafruit.io that were previously unsuccessful. Much easier than breaking it up into multiple io.send_data calls. 👍 @jerryneedell

NOTE that JSON errors can still pop up if reading back data from socket (the data was still successfully sent):

ESPSocket: 8324 bytes available
Reading 4000 bytes from ESP socket with status 4
ESPSocket: 4324 bytes available
Reading 4000 bytes from ESP socket with status 4
ESPSocket: 324 bytes available
Reading 324 bytes from ESP socket with status 4
ESPSocket: 0 bytes available
syntax error in JSON

@siddacious
Copy link
Contributor

@ladyada or @martymcguire any reason this shouldn't be merged?

@ladyada
Copy link
Member

ladyada commented Jul 11, 2019

it can be merged

@siddacious siddacious merged commit 8f964cc into adafruit:master Jul 11, 2019
@siddacious
Copy link
Contributor

@ladyada done. I can release once the server/AP PRs are merged

@ladyada
Copy link
Member

ladyada commented Jul 11, 2019

that will take a while, you can release now

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jul 12, 2019
@adafruit-adabot
Automated update by Adabot (adafruit/adabot@dfbec17)
1a6943f 
Updating https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI to 1.5.0 from 1.4.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_ESP32SPI#29 from martymcguire/mm-chunk-socket-sends

Updating https://github.com/adafruit/Adafruit_CircuitPython_SI4713 to 1.1.3 from 1.1.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_SI4713#7 from caternuson/iss2_rds

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Button to 1.1.2 from 1.1.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Button#8 from makermelissa/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_PyBadger to 0.9.0 from 0.8.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_PyBadger#3 from kattni/business-card-update
@kattni kattni mentioned this pull request Apr 29, 2020
Closed
@martymcguire martymcguire deleted the mm-chunk-socket-sends branch July 9, 2020 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ladyada ladyada Awaiting requested review from ladyada

@jerryneedell jerryneedell Awaiting requested review from jerryneedell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@martymcguire @jerryneedell @maholli @siddacious @ladyada