✨ (ble): Add ReceiveFile Service #614
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #614 +/- ##
===========================================
+ Coverage 98.18% 98.20% +0.01%
===========================================
Files 94 95 +1
Lines 1986 2004 +18
===========================================
+ Hits 1950 1968 +18
Misses 36 36
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
YannLocatelli
force-pushed
the
yann/feature/ble/receive-file-service
branch
3 times, most recently
from
da4f555
to
7cff6fc
Compare
ladislas
approved these changes
Apr 6, 2022
Comment on lines
33
to
36
| std::copy(params.data, params.data + params.len, file_reception_stream.begin() + params.offset); | ||
| if (_on_stream_callback) { | ||
| _on_stream_callback(std::span {file_reception_stream.data(), params.len}); | ||
| } |
There was a problem hiding this comment.
I haven't seen it work in real life, but seeing this makes me wonder about what happens if the consuming callback is slower than the data receive rate, let's say you want to write to a file in a low priority thread and it takes time.
I would use a circular buffer in this case so you can push data on one side and pop/consume it on the other side without impacting either one of the processes.
it could be argued that the circular buffer could be an implementation detail of the callback, but as the callback doesn't have any control over the receiving speed and process, I think it's best to play it safe and add the circular buffer here.
There was a problem hiding this comment.
Well, it works in real life
Changing the type of buffer will not change the shortcoming of data receive rate that depends only from the source. If the source set the data rate to 1MB/s, either the classical array and the circular buffer will struggle the same way.
A efficient way to handle this is to send back a signal to the source to inform that the received data has been processed.
I applied KISS for a first attempt since there was no evidence that will work and since there isn't feature in iPad to proceed it (for now).
I will change as you suggested with a circular buffer, but it will take time as it has to be tested to check that it works in real life.
There was a problem hiding this comment.
I will change as you suggested with a circular buffer, but it will take time as it has to be tested to check that it works in real life.
I did not say you must change it, just keep in mind that the issue can appear.
also, it's not about the iPad, it's about writing data to a buffer then taking the buffer to do something else.
like write data to buffer --> read buffer to write to file
if the read buffer to write to file is slow and slower than the write data, data will be corrupted and overridden each time the callback is called.
using a circular buffer prevents this because you don't write where you read, so you have time to write while you read. Sure the buffer must be big enough and if the speed difference is too big, you'll still get the issue, but at least you can check/wait if the buffer is full before writing
YannLocatelli
force-pushed
the
yann/feature/ble/receive-file-service
branch
2 times, most recently
from
0eb0027
to
e4ffb13
Compare
ladislas
reviewed
Apr 11, 2022
Comment on lines
+75
to
+84
| auto path = service_file_reception.getFilePath(); | ||
|
|
||
| if (reception_file.open(path.data(), "a")) { | ||
| write_buffer.fill('\0'); | ||
| auto data_read = file_reception_circular_queue.pop(write_buffer.data(), std::size(write_buffer)); | ||
| reception_file.write(write_buffer.data(), data_read); | ||
| reception_file.close(); | ||
| } |
There was a problem hiding this comment.
c'est cette partie là qui doit être exécutée dans un event queue afin que la partie "slow" de l'écriture ne perturbe pas la réception, que les deux soient faits dans des contexts différents.
There was a problem hiding this comment.
Je me demandais si justement ils n'était pas déjà gérés par l'eventQueue du BLE
Après ça coûte rien d'en faire un autre en effet
YannLocatelli
force-pushed
the
yann/feature/ble/receive-file-service
branch
from
b0ec0ef
to
fedd204
Compare
ladislas
approved these changes
Apr 13, 2022
YannLocatelli
force-pushed
the
yann/feature/ble/receive-file-service
branch
from
fedd204
to
e884aa7
Compare
Use CircularQueue and CoreEventQueue to transfer data from BLE to SD on receiving file
YannLocatelli
force-pushed
the
yann/feature/ble/receive-file-service
branch
from
e884aa7
to
9f35dd6
Compare
|
Kudos, SonarCloud Quality Gate passed! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.