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 parsing data dimension in lost can boards error #941

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

MSECode
Copy link
Contributor

@MSECode MSECode commented Feb 16, 2024

Fix added with this PR:

  • Remove parsing time from stillLOST canmonitor error, add parsing time to justFOUND and fix canmapping parsing Fix canmapping in can service errors
  • Fix datatype and copy-paste typo
  • Fix missing joint ID in motor generic error message

Remove parsing time from justLOST canmonitor error, add parsing time to justFOUND and fix canmapping parsing
Fix canmapping in can service errors
Fix datatype and copy-paste typo
Fix missing joint ID in motor generic error message
@MSECode MSECode requested a review from valegagge February 16, 2024 09:52
@MSECode MSECode self-assigned this Feb 16, 2024
@MSECode MSECode marked this pull request as draft February 16, 2024 09:52
@MSECode MSECode mentioned this pull request Feb 16, 2024
@pattacini
Copy link
Member

Depend on:

@valegagge
Copy link
Member

Hi @MSECode ,
Good catch! 💪
We need to be careful about the duplicated code.

I don't know which is the duplicated code SonarCloud referred to, but, in any case, is it necessary to avoid the replicated code:

            eOmn_serv_category_t serv_category =  (eOmn_serv_category_t)m_dnginfo.param16;
	        uint16_t foundMaskcan1 = (m_dnginfo.param64 & 0x00000000ffff0000) >> 16;
            uint16_t foundMaskcan2 = (m_dnginfo.param64 & 0x000000000000ffff);

            char foundCanBoards1[64] = {0};
            char foundCanBoards2[64] = {0};

            for(int i=1; i<15; i++)
            {
                if ( (foundMaskcan1 & (1<<i)) == (1<<i) )
                {
                    strcat(foundCanBoards1,  std::to_string(i).c_str());
                    strcat(foundCanBoards1, " ");
                }

                if ( (foundMaskcan2 & (1<<i)) == (1<<i) )
                {
                    strcat(foundCanBoards2,  std::to_string(i).c_str());
                    strcat(foundCanBoards2, " ");
                }
            }

            snprintf(str, sizeof(str), "%s Type of service category is %s. CAN boards are on (can1map, can2map) = ([ %s ], [ %s ])",
                m_dnginfo.baseMessage.c_str(),
                eomn_servicecategory2string(serv_category),
                foundCanBoards1,
                foundCanBoards2
            );
            m_dnginfo.baseInfo.finalMessage.append(str);

We ought to create a function that parametrizes the string to print in case of justlost, stilllost, etc..

I guess we can use the same function in the cases eoerror_value_SYS_canservices_boards_lostcontact and eoerror_value_SYS_canservices_boards_retrievedcontact because currently they have the same output string and it is necessary to disambiguate the two cases.

@MSECode MSECode force-pushed the fix/diagnosticLostCANBoards branch from 08bba06 to 09e3d46 Compare February 19, 2024 14:41
@valegagge
Copy link
Member

Hi @MSECode ,
we could improve a bit more in this way:

const int maxstr =64; 
typedef char miastr[maxstr];



void MotionControlParser::miaFunz(uint16_t foundMaskcan, miastr64 foundCanBoards)
{
    for(int i=1; i<15; i++)
    {
        miastr b{};
        if ( (foundMaskcan1 & (1<<i)) == (1<<i) )
        {
            //std::to_chars(aux, aux+2, i);
            snprintf(b, maxstr, "%d ", i);
            strcat(foundCanBoards, b);
        }
    }

}

MotionControlParser::new_private_function (eOmn_serv_category_t serv_category, miastr64 foundCanBoards1, miastr64 foundCanBoards2)
{

            eOmn_serv_category_t serv_category =  (eOmn_serv_category_t)m_dnginfo.param16;
	        uint16_t foundMaskcan1 = (m_dnginfo.param64 & 0x00000000ffff0000) >> 16;
            uint16_t foundMaskcan2 = (m_dnginfo.param64 & 0x000000000000ffff);

            miaFunz(foundMaskcan1, foundCanBoards1);
            miaFunz(foundMaskcan2, foundCanBoards2);
    

}

In this way, we reuse the code totally and provide a bit safer code forcing the use of the sting with a fixed size. Moreover, we avoid creating a temporary object on the heap with the to_string function.
What do you think?

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
10.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

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.

well done!
Test already done! see here see #937

@valegagge valegagge marked this pull request as ready for review February 21, 2024 21:10
@pattacini pattacini marked this pull request as draft February 22, 2024 08:17
@valegagge
Copy link
Member

valegagge commented Feb 22, 2024

This PR is in draft, because we are waiting for the robotology/icub-firmware-shared#92.

cc @pattacini @MSECode @marcoaccame

@pattacini
Copy link
Member

robotology/icub-firmware-shared#92 is merged.
Putting this PR in ready for review to enable CI.

@pattacini pattacini marked this pull request as ready for review February 23, 2024 11:51
@pattacini
Copy link
Member

CI is ok ✅
Merging.

@pattacini pattacini merged commit c26771a into robotology:devel Feb 23, 2024
4 of 13 checks passed
@MSECode MSECode deleted the fix/diagnosticLostCANBoards branch March 6, 2024 08:49
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.

Fix diagnostic messages
3 participants