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

simulator move to PX4Accelerometer, PX4Gyroscope, PX4Magnetometer, PX4Barometer helpers #12081

Merged
merged 1 commit into from
May 31, 2019

Conversation

dagar
Copy link
Member

@dagar dagar commented May 26, 2019

The sim drivers are a good idea, but the implementation is a bit of a mess and they're getting in the way of larger sensor pipeline changes and dropping DriverFramework. For now I propose dropping them and simply using the helper classes (PX4Accelerometer, PX4Gyroscope, etc) directly in the simulator.

Not only is this much simpler, but it keeps the details in sync with what the real drivers are actually doing (filting, integration, respecting parameters IMU_GYRO_CUTOFF, etc).

Longer term I'd like to resurrect the sim driver idea, but instead do it as a simulation interface to a real driver.

@dagar
Copy link
Member Author

dagar commented May 27, 2019

The tiltrotor SITL failure is real, but I think it's more along the lines of not letting something sketchy in that gazebo model slip through anymore with this change.

@dagar
Copy link
Member Author

dagar commented May 27, 2019

Let's disable tiltrotor for now until someone can take a closer look. It's failing quite often in other PRs and master as well. #12085

@julianoes
Copy link
Contributor

@dagar does the CI fail happen only with these changes or in general ?

@dagar
Copy link
Member Author

dagar commented May 27, 2019

@dagar does the CI fail happen only with these changes or in general ?

In general. PR rebased on master (with tiltrotor temporarily disabled).

src/modules/simulator/simulator_mavlink.cpp Outdated Show resolved Hide resolved
@@ -166,11 +126,6 @@ int Simulator::start(int argc, char *argv[])
_instance->poll_for_MAVLink_messages();
#endif

} else if (argv[2][1] == 'p') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this breaks HITL for Raspberry Pi or something like that but I'm not sure anymore.

Copy link
Member Author

@dagar dagar May 30, 2019

Choose a reason for hiding this comment

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

Shouldn't matter anymore without the need for most of the simulated sensors.

src/modules/simulator/simulator.cpp Show resolved Hide resolved
src/modules/simulator/simulator_mavlink.cpp Show resolved Hide resolved
src/modules/simulator/simulator_mavlink.cpp Outdated Show resolved Hide resolved
@julianoes
Copy link
Contributor

The problem with this change is that we can no longer simulate sensor failover which is something we should do. However, if this change means it's easier to refactor sensor drivers and to remove the DriverFramework, then I'm ok with it for now.

@dagar
Copy link
Member Author

dagar commented May 28, 2019

The problem with this change is that we can no longer simulate sensor failover which is something we should do. However, if this change means it's easier to refactor sensor drivers and to remove the DriverFramework, then I'm ok with it for now.

Yes I'm not happy about that part either, but realistically we need to do a lot more than simulate the data stopping.

julianoes
julianoes previously approved these changes May 28, 2019
@dagar dagar force-pushed the pr-sim_driver_cleanup branch 3 times, most recently from 36aa412 to 78b659d Compare May 30, 2019 13:12
@dagar dagar mentioned this pull request May 30, 2019
19 tasks
@dagar dagar merged commit 43e3fc7 into PX4:master May 31, 2019
@dagar dagar deleted the pr-sim_driver_cleanup branch May 31, 2019 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants