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

changed structure in od_angular.h #407

Closed
wants to merge 5 commits into from

Conversation

Faizan711
Copy link

@Faizan711 Faizan711 commented Dec 3, 2022

@Simonbdy submiting this PR, you agree with the associated license (Apache 2.0 or MIT) and with our Contributor License Agreement (CLA).

Before to begin

Thank you for contributing to the Luos project!

Before to begin, please follow these steps:

  • Ensure that this PR is not a duplicate.
  • Assign @Simonbdy to this PR
  • Add the PR to the Luos contribution project

Feel free to read the Luos contribution's guidelines and the documentation page to have more insight about how to contribute to Luos.

PR Description section

This PR is regarding a change in structure mentioned in issue #295.

Description and dependencies

Please include here a summary of the changes and the related issue. List any dependencies that are required for this change.
Here i have changed the typedef structure to struct only in the od_angular.h file in OD directory and i want you to verify the change , so i can make the changes to the rest of the files in the OD directory if correct. If something is wrong i will modify it accordingly.

Changes

Please choose the relevant options:

  • New feature (non-breaking change which adds #functionality)

Related issue(s)

Provide a list of the related issues that will be fixed by this PR.

issue number #295

WARNING: Do not edit the checklist below.


Developer section

  • [Documentation] is up to date with new feature
  • [Tests] are passed OK (non regression, new features & bug fixes)
  • [Code Quality] please check if:
    • Each function has a header (description, inputs, outputs)
    • Code is commented (particularly in hard to understand areas)
    • There are no new warnings that can be corrected
    • Commits policy is respected (constitancy commits, clear commits comments)

QA section

  • [Review] tests for new features have been reviewed
  • [Changelog] is up-to-date with expected tags
    🆕 Feature: [Feature] Description...
    🆕 Added: [Feature] Description...
    🆕 Changed: [Feature] Description...
    🛠️ Fix: [Feature] Description...

@nicolas-rabault nicolas-rabault self-requested a review December 4, 2022 10:58
Copy link
Member

@nicolas-rabault nicolas-rabault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!
I just spotted a strange line and some clarity improvement.
Also perhaps there is some cast missing, we will have to watch for compilation warnings...
I also enable the bot, so you can consult the result on the bottom.

engine/OD/od_angular.h Outdated Show resolved Hide resolved
engine/OD/od_angular.h Outdated Show resolved Hide resolved
engine/OD/od_angular.h Outdated Show resolved Hide resolved
engine/OD/od_angular.h Outdated Show resolved Hide resolved
engine/OD/od_angular.h Outdated Show resolved Hide resolved
@nicolas-rabault nicolas-rabault changed the base branch from main to rc_2.8.0 December 4, 2022 11:06
@Faizan711
Copy link
Author

should i close this PR and make a PR again after correcting my mistakes??

@Simonbdy
Copy link
Contributor

Simonbdy commented Dec 5, 2022

should i close this PR and make a PR again after correcting my mistakes??

@Faizan711, you can make as many loops and corrections as needed in this PR. No need to open a new PR for each correction. Eventually when everything is smooth and reviewed we will merge it.

I have changed self.private to self as mentioned and also corrected the line mistake mentioned.
@Faizan711
Copy link
Author

@Simonbdy , @nicolas-rabault I have made the corrections mentioned. Please check this and let me know if its correct.

Copy link
Member

@nicolas-rabault nicolas-rabault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Watch out not everything should be just self. When you return float values you need to use self.private because it's a float value.

engine/OD/od_angular.h Outdated Show resolved Hide resolved
engine/OD/od_angular.h Outdated Show resolved Hide resolved
engine/OD/od_angular.h Outdated Show resolved Hide resolved
changed "self.private" for float values and "self" for rest.
@Faizan711
Copy link
Author

@nicolas-rabault I have made the corrections for float values, please check and let me know if it is correct, if not i will make the corrections again.

@nicolas-rabault nicolas-rabault requested review from nicolas-rabault and removed request for nicolas-rabault December 7, 2022 13:52
@nicolas-rabault
Copy link
Member

You should try to compile a code example using this code and look at the compilation errors to check it out first.
Our GitHub action bot spotted some mistakes here : https://github.com/Luos-io/luos_engine/actions/runs/3639529878/jobs/6143038302
You can try to compile examples/projects/native/button/ project. This is a simple one able to run on your computer.

@Faizan711
Copy link
Author

OK i will try to compile, if i get issues i will connect with you here or in discord.

@nicolas-rabault
Copy link
Member

For compilation issue Discord is more appropriate, because it's (probably) not directly linked to this particular PR.

@Faizan711
Copy link
Author

Sorry @nicolas-rabault i was inactive for few days due to my exams. I need some help on how to run the updated script. I also dropped a message in discord but didn't got any response, can you please provide some help?

@nicolas-rabault
Copy link
Member

I don't know what you mean by updated script.

@Faizan711
Copy link
Author

Sorry i was unclear, i meant the file in which i have made changes. The one named od_angluar.h

@nicolas-rabault
Copy link
Member

By modifying the file on your local version of Luos_engine and compile the native example I mention Platformio will compile your modified file and display your potential mistakes.

@JeromeGalan JeromeGalan changed the base branch from rc_2.8.0 to rc_2.8.1 December 16, 2022 18:10
@JeromeGalan JeromeGalan added this to the 2.8.1 milestone Dec 16, 2022
@nicolas-rabault nicolas-rabault requested review from nicolas-rabault and removed request for nicolas-rabault January 5, 2023 16:02
@JeromeGalan JeromeGalan removed this from the 2.8.1 milestone Jan 9, 2023
@JeromeGalan JeromeGalan changed the base branch from rc_2.8.1 to rc_2.9.0 February 14, 2023 09:04
@nicolas-rabault
Copy link
Member

This Issue have been addresed already in 2.9.0

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

Successfully merging this pull request may close these issues.

[NEW FEATURE] Secure OD structure usage avoiding users to directly set it
4 participants