Open ddmesh opened 2 months ago
CLAassistant
commented
2 months ago
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
github-actions[bot]
commented
2 months ago Memory usage change @ ef496059019cfb51a002c9d18c74a553df6fac90
| Board | flash | % | RAM for global variables | % |
|---|---|---|---|---|
arduino:mbed_opta:opta |
:small_red_triangle: +176 - +184 | +0.01 - +0.01 | 0 - 0 | 0.0 - 0.0 |
arduino:mbed_portenta:envie_m7 |
N/A | N/A | N/A | N/A |
arduino:renesas_portenta:portenta_c33 |
:small_red_triangle: +128 - +200 | +0.01 - +0.01 | 0 - 0 | 0.0 - 0.0 |
sebromero
commented
2 months ago Thanks a lot @ddmesh for your contribution! We'll take a look at your PR shortly 👀
facchinm
commented
2 months ago Hi @ddmesh, thanks for the patch, but in its current state it is unmergeable. I would kindly ask you to split into different commits with clean scope (for example, one for typos, one for functionalities, one for bugfixes) without unrelated whitespace changes, all with a clear description of what they are doing.
ddmesh
commented
2 months ago Hi, I can not split this commit into several. There are not so much changes and those should be checked easily. Most code editors like VSCode will remove spaces automatically to avoid more visible changes in future in source code. So it is always a good idea to never check-in code that has spaces at line end.
Please compare the code with spaces ignored. We have our own git repository in our company where I have fixed those issues. I just wanted to give those fixes back and created a commit for you. I took me already quite a while to analyse and test the code after I run into unexpected issues. I really would appreciate that you verify and check the merge request as it is. I can not separate this commit (no time).
Git CI tells that all tests are successful, only one reviewer is needed. The code is mergable, as there are no merge conflicts. All changes are on top on main branch.
facchinm
commented
2 months ago @ddmesh can you sign the CLA please? I'll then proceed in splitting the PR into sensible commits to keep the actual patch understandable for future readers :slightly_smiling_face:
ddmesh
commented
2 months ago @ddmesh can you sign the CLA please? I'll then proceed in splitting the PR into sensible commits to keep the actual patch understandable for future readers 🙂
hi, thanks a lot for splitting. I have signed the cla several times and it says that I already have it. but the state here is not updated. on cla website it is also listed as "signed". seems GitHub integration is not working correctly. BR Stephan
facchinm
commented
2 months ago I think the CLA is failing due to the committer being dummy@github.com . I splitted between whitespaces fixes and actual code, and I noticed a thing that don't sound completely right:
this -> blockDevice = BlockDeviceType::get_default_instance(); this -> mbrBlockDevice = new MBRBlockDeviceType(this -> blockDevice, this->partitionNumber); from the constructor can have unintended consequences due to the fact that the constructors might be called in any order. Why did you move it from begin()?The other modifications look sensible, anyway I'd ask @aliphys and @cristidragomir97 to give it a spin before merging
ddmesh
commented
2 months ago I moved the creation/initialisation from begin to constructor because the blockdevice is global to "InternalStorage" and does not change at all over time. Only when the InternalStorage object is destroyed the device below becomes released. begin() is used to mount a file system. The user can umount/mount his blockdevice as he likes several times. For instance do an umount/format/mount. The whole time the block device should stay initalised and must not initialised more then once. If you always destroy and reinit the underlaying blockdevice than you waste a lot of time/resources which can be easily avoided.
There is a 1:1 relation how long InternalStorage object lives and the lifetime of underlaying blockdevice.
When user creates an instance of InternalStorage globally, you are right, that the order when a constructor is called is undefined. But a conflict with the underlaying blockdevice (e.g. QSPI flash) should not happen at all, because only one "owner" must initialise the underlaying blockdevice. Using different APIs or create more than one instance are a programming error. When you initialize the underlaying blockdevice in begin() you will have the same issue and more, beause you reinitialize the blockdevice several times without freeing it (memleak and deadlock). If you want to protect the InternalStorage object, you must create a singleton , so only one instance of InternalStorage can be created at all.
github-actions[bot]
commented
2 months ago Memory usage change @ bd8600ccc0c454631cc41bf2928c6c80b0bc95de
| Board | flash | % | RAM for global variables | % |
|---|---|---|---|---|
arduino:mbed_opta:opta |
:small_red_triangle: +192 - +192 | +0.01 - +0.01 | 0 - 0 | 0.0 - 0.0 |
arduino:mbed_portenta:envie_m7 |
N/A | N/A | N/A | N/A |
arduino:renesas_portenta:portenta_c33 |
:small_red_triangle: +136 - +208 | +0.01 - +0.01 | 0 - 0 | 0.0 - 0.0 |
ddmesh
commented
2 months ago About the CLA, I tried to change "amend" the author from last commit, but his was ignored by github after pushing it (if this is the reason why the status is still "pending"). Any Idea?
beside of this, when commiting and pushing to github, github replaces the email address anyway to protect privacy. Perhaps there is another reason why this is pending?
I had a lot of instabilities while testing and evaluating this library and found a lot of issues that should be fixed in main stream.