Skip to content

Add MBD-based FT FW #180

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

Merged
merged 9 commits into from
Apr 27, 2021
Merged

Add MBD-based FT FW #180

merged 9 commits into from
Apr 27, 2021

Conversation

triccyx
Copy link
Member

@triccyx triccyx commented Apr 13, 2021

Used MBD generated code from Simulink as new FT sensor code.
Check Simulink model in robotology/icub-firmware-models#2.

@pattacini pattacini changed the title Mbd ft Add MBD-based FT FW Apr 13, 2021
@pattacini pattacini marked this pull request as draft April 13, 2021 08:30
@pattacini pattacini requested a review from marcoaccame April 13, 2021 08:37
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.

I took the chance to trace back the pristine memory areas from which we do the memcpy:

embot::dsp::q15::matrix transfQ15matrix[numOfSets];
embot::dsp::q15::matrix transfQ15tare[numOfSets];

Then, it'd be sufficient to move the memcpy into those code lines where transfQ15matrix and transfQ15tare are filled in to obtain the correct asynchronous update.

However, I see (and I remember now) that the FW deals with a set of diverse matrices and offsets; therefore, it might be convenient to keep things as they are now, for the sake of simplicity.

After all, the continuous copy does not take long.

@triccyx
Copy link
Member Author

triccyx commented Apr 14, 2021

... sake of simplicity

Agree

@pattacini pattacini marked this pull request as ready for review April 14, 2021 19:53
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.

@simeonedussoni and @triccyx tested the MBD FW today successfully 🚀

Hence, @marcoaccame you can now review the code.

@marcoaccame
Copy link
Contributor

Hence, @marcoaccame you can now review the code.

Hi @pattacini and @triccyx, I'm having a look at it.

Copy link
Contributor

@marcoaccame marcoaccame 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: the changes are OK and can be merged.

There are only some minor comments reported in the following.

  • Inside bool embot::app::application::theSTRAIN::Impl::processing() you moved the calibration inside if(false == runtimedata.data.adcsaturation) { .. } . It was originally outside. Does this movement of code change the behaviour? I have checked. The behaviour is the same. I am telling that only because at first I thought there was a mistake. But it there is not any.

  • When you produce a new binary, you will need to advance the application number so that we can identify the new binary vs current released version which now is:

    constexpr embot::app::theCANboardInfo::applicationInfo applInfo 
    { 
        embot::prot::can::versionOfAPPLICATION {2, 0, 10},
        embot::prot::can::versionOfCANPROTOCOL {2, 0}    
    };
  • In file embot_app_application_theSTRAIN.h I see an include of a .h file w/ the entire relative path. That compiles and is fine, but it is better to remove the path in inclusions and to add it to the environment variable of the project.

  • Your fork is not rebased vs latest robotology:devel. However, the changes of the missing PRs are not related to any of the code you have touched and automatic rebase will work fine.

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.

In file embot_app_application_theSTRAIN.h I see an include of a .h file w/ the entire relative path. That compiles and is fine, but it is better to remove the path in inclusions and to add it to the environment variable of the project.

I agree w/ @marcoaccame.
@simeonedussoni @triccyx could you address this, please?

@simeonedussoni
Copy link
Contributor

fixed few minor issues:

  • version number is now 2.0.13 (note: it is already higher than the IMU remap)
  • tabs are replaced with spaces
  • included file is without explicit path, which is in the configuration tab

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.

Thanks @triccyx @simeonedussoni 🚀
Let's proceed with yielding the binaries then!

@pattacini pattacini merged commit 262fae7 into robotology:devel Apr 27, 2021
@simeonedussoni simeonedussoni deleted the mbd-ft branch April 27, 2021 11:35
marcoaccame pushed a commit to robotology/icub-firmware-build that referenced this pull request May 3, 2021
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.

None yet

4 participants