Enlarge the chunkSize #75
Closed
Conversation
This file contains hidden or 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
|
|
|
Memory usage change @ 89f347b
Click for full report table
Click for full report CSV |
|
Memory usage change @ c2e55e7
Click for full report table
Click for full report CSV |
This was referenced Jul 22, 2021
|
Hi @CptHolzschnauz. Thanks so much for taking the time to submit a pull request. It seems like there are a lot of changes being added here that aren't related to the stated purpose of the pull request: "Enlarge the chunkSize". I just wanted to check in: are you intended to push all these commits to the pull request? What is maybe a bit surprising at times is that, even after you have submitted a pull request, any commit you push to that branch of your fork is added to the pull request. The pull request is not a snapshot of the branch at the time it was submitted. Because we may want to continue with unrelated development on our forks, it is usually best practices to create a dedicated "feature branch" of the fork for each pull request. I see this pull request was submitted from the |
|
Hi Per1234
No, sorry, i forgot, my mistake. I’m writing a class for the internal mqtt functionality of the SARA410 modem at the moment and forgot about my chunkSize PR and started the other construction site on the same branch.
Now it’s a bit a mess…sorry. The chunkSize enlargement is a small change, not really worth a PR as you can see in NBClient.cpp, 288:
command.reserve(19 + (size > 512 ? 512 : size) * 2);
while (size) {
size_t chunkSize = size;
if (chunkSize > 512) {
chunkSize = 512;
}
From 256 to 512 because the SARA can handle more than the U201 from the MKR1400. I tested it and it worked, but it would be better a pro double checks the change.
Because of that little change I didn’t made a feature branch
The new NBMQTT class is far away from ready to be merged and I didn’t want to PR these files, it was my mistake as you described it…
Now I don’t know what’s the best solution for this mess, should i revoke the PR?
Thx for your indulgence for beginners…
Regards
… Am 04.08.2021 um 22:47 schrieb per1234 ***@***.***>:
Hi @CptHolzschnauz <https://github.com/CptHolzschnauz>. Thanks so much for taking the time to submit a pull request. It seems like there are a lot of changes being added here that aren't related to the stated purpose of the pull request: "Enlarge the chunkSize". I just wanted to check in: are you intended to push all these commits to the pull request?
What is maybe a bit surprising at times is that, even after you have submitted a pull request, any commit you push to that branch of your fork is added to the pull request. The pull request is not a snapshot of the branch at the time it was submitted. Because we may want to continue with unrelated development on our forks, it is usually best practices to create a dedicated "feature branch" of the fork for each pull request. I see this pull request was submitted from the master branch of your fork, so any commit pushed to that master branch becomes part of this pull request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#75 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQME3CZLCDRXYAMKIR775JTT3GRNPANCNFSM42YVAUQA>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>.
|
PRs for small, self-contained changes are welcome.
From the perspective of the PR alone, the ideal solution is for you to do a hard reset of your repository's From a quick look, I believe the last on-topic commit was this one: HOWEVER, be warned that this will cause all the advancements you have made in the Note that changing the history of a repository via force pushes is not generally a good practice, since this can affect people who are relying on the branch's history to be inviolate. But in the special case of a "feature branch" that is only ever intended to be used to produce a pull request, it is generally considered acceptable, and even preferred to do force pushes as is required by ongoing development of the pull request proposal (e.g., in response to reviewer feedback) in order to avoid cluttering up the upstream repository's commit history with those "fixup" commits should be PR be merged. |
|
To prevent a git mess.. |
|
I set to draft and closed the PR. Otherwise it’s a mess. My plan was only a small change and so I didn’t set up a proper local clone etc.
Thx anyway
… Am 05.08.2021 um 00:29 schrieb per1234 ***@***.***>:
The chunkSize enlargement is a small change, not really worth a PR
PRs for small, self-contained changes are welcome.
Now I don’t know what’s the best solution for this mess, should i revoke the PR?
From the perspective of the PR alone, the ideal solution is for you to do a hard reset of your repository's master branch back to the last commit that was intended to be added to the PR, then do a force push to your fork here on GitHub.
From a quick look, I believe the last on-topic commit was this one:
c2e55e7 <c2e55e7>
If so, the commands would be:
git clone https://github.com/CptHolzschnauz/MKRNB
cd MKRNB
git reset --hard c2e55e7
git push --force-with-lease
HOWEVER, be warned that this will cause all the advancements you have made in the master branch to be lost, so it might not be the ideal solution as far as your work outside the scope of this PR is concerned. What you could do is to create a branch off of master before you reset it. That will preserve all the work you have currently in master in that other branch.
Note that changing the history of a repository via force pushes is not generally a good practice, since this can affect people who are relying on the branch's history to be inviolate. But in the special case of a "feature branch" that is only ever intended to be used to produce a pull request, it is generally considered acceptable, and even preferred to do force pushes as is required by ongoing development of the pull request proposal (e.g., in response to reviewer feedback) in order to avoid cluttering up the upstream repository's commit history with those "fixup" commits should be PR be merged.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#75 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQME3CY2TYNBHITH7LFK4UTT3G5OLANCNFSM42YVAUQA>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
My problem: Because the chunkSize is limited to 256, the lib writes two times into the socket when the MQTT message is larger which causes problems with my mqtt broker.
The modem is capable to handle more, see:
AT+USOWR=?
+USOWR: (0-6),(0-512),"HEX data"
+USOWR: (0-6),(0-1024),"data"
So i suggest to enlarge the chunkSize at least up to 512 to write more data into the socket in one step.
This should improve the performance for all users because of less overhead data and less AT commands to the modem.
This lib is more or less a copy of MKRGSM and needs some fine tuning for the SARA modem.
Dear coding cracks, please put some love in this very important lib!