Skip to content
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.

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

Stsck mem optimization for sha256 hash generation #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

toshiharutf
Copy link

@toshiharutf toshiharutf commented Feb 3, 2022

Made buffers in mavlink_sha256_calc thread_local to save space in stack.

@toshiharutf toshiharutf changed the title RAM optimization for sha256 hash generation Stsck mem optimization for sha256 hash generation Feb 3, 2022
@peterbarker
Copy link

This PR is against a repository which contains generated commits; it should be against the generator instead.

One issue with that patch is that I don't believe we currently have a C11 prerequisite for compilation - and this would introduce that.

@toshiharutf
Copy link
Author

Hello Patrick,
thanks for your time to review my PR. It's my first developing for mavlink. Is mavlink generator a github pipeline? is that related to https://hamishwillee.gitbooks.io/ham_mavdevguide/content/en/ ? Where should I create the PR then?

Also, according to this doc https://mavlink.io/en/guide/mavlink_2.html, mavlink2 should be C++11 compliant. I could use static, instead of thread_local, but it would only be safe to use for single threaded applications.

Bests,

@hamishwillee
Copy link

hamishwillee commented Mar 16, 2022

@peterbarker The generator separately supports generation of Cpp11 libraries from the normal C library in this repo. So in theory we'd accept a generator fix that made that version more compliant. But as you say, not this one.

@toshiharutf Firstly, thanks for contributing :-). I presume you are using this library in anger?

Also, according to this doc https://mavlink.io/en/guide/mavlink_2.html, mavlink2 should be C++11 compliant. I could use static, instead of thread_local, but it would only be safe to use for single threaded applications.

No, according to that statement MAVLink supports C++11 compliant libraries. That's true, but it isn't this library. This library is generated using --lang=c (for generator docs ).

What @peterbarker is saying is that this library is generated from source files somewhere else. So you need to update the originals or the change would be swamped next time we update the library. The original file, where a PR would be required is of this change is : https://github.com/ArduPilot/pymavlink/blob/master/generator/C/include_v2.0/mavlink_sha256.h

In summary, you would need to run the generator and see what is generated. If it is the file https://github.com/ArduPilot/pymavlink/blob/master/generator/C/include_v2.0/mavlink_sha256.h then that would mean it is shared by both builds - so you'd have to either conditionally build your changes or split the file.

I'm going to close this as invalid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants