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

Fix/diagnostic gen error #942

Merged
merged 1 commit into from
Mar 14, 2024
Merged

Conversation

MSECode
Copy link
Contributor

@MSECode MSECode commented Feb 21, 2024

Updates for generic error translation from bits to string using this method motorStatusBitsToString

correlated to: #941 since it's rebased on that and it should be merged after
and it is directly related to: robotology/icub-firmware#472

Related to:
robotology/icub-firmware-shared#94
robotology/icub-firmware#472

@MSECode MSECode self-assigned this Feb 21, 2024
@MSECode MSECode marked this pull request as draft February 21, 2024 07:45
@pattacini
Copy link
Member

Hi @MSECode

Is this linked to #937?
Please, don't open up PR with a blank body 😉

@MSECode
Copy link
Contributor Author

MSECode commented Feb 21, 2024

Yes
I know, I have to update it. I put in draft in the meanwhile as a placeholder

@valegagge
Copy link
Member

Hi @MSECode,
I think here you could use the string_view for your map.
Moreover, concatenating the strings in this way means a lot of heap allocation.
We need to try to reduce them. We can discuss how to do it.
strcat is not the best way for safety reasons, but we can find a good solution for it.

PS: I know in the diagnostic parser I used some strings, but I want to improve it to reduce the heap usage anyway.

@MSECode
Copy link
Contributor Author

MSECode commented Feb 23, 2024

let's add that point to the f2f discussion as well
So, we could do something like this:
constexpr std::string_view s_motor_fault_status[]{string1, string2, ....} so that it would also be evaluated at compile time

@pattacini pattacini linked an issue Feb 23, 2024 that may be closed by this pull request
@pattacini
Copy link
Member

pattacini commented Feb 23, 2024

pattacini

This comment was marked as resolved.

@pattacini

This comment was marked as resolved.

@pattacini pattacini linked an issue Mar 4, 2024 that may be closed by this pull request
@MSECode MSECode force-pushed the fix/diagnosticGenError branch 2 times, most recently from 69f1446 to a3b9c8c Compare March 6, 2024 08:53
@pattacini pattacini linked an issue Mar 6, 2024 that may be closed by this pull request
Copy link
Member

@valegagge valegagge left a comment

Choose a reason for hiding this comment

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

LGTM

@MSECode MSECode marked this pull request as ready for review March 13, 2024 15:17
@MSECode MSECode marked this pull request as draft March 13, 2024 15:26
@MSECode MSECode marked this pull request as ready for review March 13, 2024 16:22
@MSECode
Copy link
Contributor Author

MSECode commented Mar 14, 2024

I need to update the CMakeLists.txt adding icub_firmware_shared::embot to fix compilation

Copy link

sonarcloud bot commented Mar 14, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
10.1% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link
Member

@pattacini pattacini left a comment

Choose a reason for hiding this comment

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

Awaiting the CI before merging.

@MSECode
Copy link
Contributor Author

MSECode commented Mar 14, 2024

Awaiting the CI before merging.

Ok, understood. I leave it running since I did not get any building error locally (since the compiler was probably able to find the embot library as installed on my system ) and I realized the missing of it just from the previous failing of the CI.
Is there a best practice I can use to simulate the job of the CI and so find any possible building error also locally (I'm thinking if could be useful in this case to use a virgin docker image but let me know what should be the best)?

Another point: regarding the Quality Gate. How should I consider the lines of code interested by a failing in the code analysis that have not been touched by the changes on the PR but still checked by SonarCloud?

Update Generic error parsing adding common string map

Add icub_firmware_shared::embot lib to embObjLib thus to use embot::core::binary::bit::check
Update include array
@pattacini
Copy link
Member

Is there a best practice I can use to simulate the job of the CI and so find any possible building error also locally (I'm thinking if could be useful in this case to use a virgin docker image but let me know what should be the best)?

Docker images are a possibility but somewhat limited to linux envs.
On Windows, there are maybe alternatives to create testing environments locally. One solution could be relying on Windows Sandbox, which in turn can be configured for the problems at hand.

Another point: regarding the Quality Gate. How should I consider the lines of code interested by a failing in the code analysis that have not been touched by the changes on the PR but still checked by SonarCloud?

We're not handling SonarCloud: no worries

@pattacini pattacini merged commit 59f5b7b into robotology:devel Mar 14, 2024
4 checks passed
@MSECode MSECode deleted the fix/diagnosticGenError branch March 14, 2024 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants