Skip to content

Chunk buffer sends into 64 byte chunks #29

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
jerryneedell
jerryneedell reviewed Mar 13, 2019
View reviewed changes
@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
# for free to join this conversation on GitHub. Already have an account? # 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