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 px4 connection for wsl 2. #3603

Merged
merged 44 commits into from
May 6, 2021
Merged

fix px4 connection for wsl 2. #3603

merged 44 commits into from
May 6, 2021

Conversation

lovettchris
Copy link
Member

@lovettchris lovettchris commented Apr 16, 2021

Fixes: #
many issues search for PX4

About

AirSim cannot talk to PX4 on WSL 2 because WSL 2 does not use localhost. This adds support for
AirSim to talk to PX4 running on remote IP, where PX4 is configured to find AirSim using the
new PX4_SIM_HOST_ADDR environment variable (need latest PX4 master bits).

Also contains a fix that makes GPS fusion and Home Position happen a lot faster.

How Has This Been Tested?

windows + wsl 2 px4 firmware connect AirSim with these settings:

            "ControlPortLocal": 14540,
            "ControlPortRemote": 14580,
            "ControlIp": "remote",
            "LocalHostIp": "172.24.240.1",
            "Parameters": {
                "NAV_RCL_ACT": 0,
                "NAV_DLL_ACT": 0,
                "LPE_LAT": 47.641468,
                "LPE_LON": -122.140165,
                "COM_OBL_ACT": 1
            },

cd PythonClient\multirotor
python stability_test.py

ran for 1 hour not finding any problems.
also tested that the same works with cygwin version of px4 running on localhost with these settings:

            "ControlPortLocal": 14540,
            "ControlPortRemote": 14580,
            "ControlIp": "127.0.0.1",
            "LocalHostIp": "127.0.0.1",

and also tested Pixhawk HITL with serial port and all 3 tests worked fine.

Here is a cooked AirSimNHTest binary containing these bits.

Screenshots (if appropriate):

@jonyMarino jonyMarino added the px4 label Apr 19, 2021
@rajat2004
Copy link
Contributor

rajat2004 commented Apr 27, 2021

Also, I'm currently trying to figure out some strange GPS readings I'm seeing with ArduPilot, see #3364 (comment) and below, is something like this occurring with PX4 as well? Continuous ascend is enough to always trigger this, around 50m

@lovettchris
Copy link
Member Author

@rajat2004, regarding GPS signal, I am not seeing what you mention in #3364. I will add comments on that PR rather than here as that seems to be an unrelated topic

@rajat2004
Copy link
Contributor

rajat2004 commented Apr 28, 2021

Ah, #3364 did not fit cleanly with this, sorry for that.
Edit, so merge got committed just as I commented

Also, there's a statement (L1137 in the new code of this PR) -

else if (mav_vehicle_ != nullptr) {
mav_vehicle_->getConnection()->startLoggingSendMessage(std::make_shared<MavLinkLogViewerLog>(logviewer_out_proxy_));

This is starting logging on a different log file, but the same connection logger gets overwritten in the onArmed() method. This will likely happen before the vehicle gets armed, so maybe the earlier stuff gets logged? But there's actually no log file being opened here also (openForWriting)

The corresponding stop logging (L881 now) -

if (mav_vehicle_ != nullptr) {
auto c = mav_vehicle_->getConnection();
if (c != nullptr) {
c->stopLoggingSendMessage();
}
mav_vehicle_->close();

@lovettchris
Copy link
Member Author

@rajat2004, I wonder if github can do code reviews with comments on the actual line of code where you think there's a problem. Might be easier for context that way. But Yes, this is doing realtime LogViewer logging and that is mutually exclusive with file logging.

mav_vehicle_->getConnection()->startLoggingSendMessage(std::make_shared<MavLinkLogViewerLog>(logviewer_out_proxy_)); 

I pushed a fix to this to make it more consistent and added docs to make it clear that these are mutually exclusive.

@rajat2004
Copy link
Contributor

Makes sense if the 2 loggers are mutually exclusive. I would have commented on the specific line itself but Github doesn't allow comments on code which hasn't changed if it's more than some 10 lines or so away from any changes.

@lovettchris
Copy link
Member Author

got it, yeah, I still like the devops code review system a lot better than github...

Copy link
Contributor

@zimmy87 zimmy87 left a comment

Choose a reason for hiding this comment

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

Mostly some comments on style and typos to conform to the coding guidelines

MavLinkCom/include/MavLinkMessageBase.hpp Outdated Show resolved Hide resolved
PythonClient/multirotor/speaker.py Outdated Show resolved Hide resolved
PythonClient/multirotor/wav_reader.py Show resolved Hide resolved
build.cmd Outdated Show resolved Hide resolved
docs/log_viewer.md Outdated Show resolved Hide resolved
docs/px4_setup.md Outdated Show resolved Hide resolved
Copy link
Contributor

@zimmy87 zimmy87 left a comment

Choose a reason for hiding this comment

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

extra comment caught after previous review

Copy link
Contributor

@zimmy87 zimmy87 left a comment

Choose a reason for hiding this comment

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

Some additional comments from doing testing on this PR

AirLib/include/sensors/lidar/LidarSimple.hpp Show resolved Hide resolved
docs/px4_logging.md Show resolved Hide resolved
mkdocs.yml Show resolved Hide resolved
…cessary metadata from mavlink messages and make tooltips show on top of close box in the charts.
@zimmy87
Copy link
Contributor

zimmy87 commented May 6, 2021

Tested repro of #3415, python scripts that were changed, and the Unity build. #3415 didn't occur over 3 hour-long test runs in WSL2 (it still appears to repro in Cygwin, but I think that is better addressed in a separate PR), and I didn't observe any regressions elsewhere, so I am moving ahead with merging this.

@zimmy87 zimmy87 merged commit 8d21f60 into master May 6, 2021
@lovettchris lovettchris deleted the clovett/mavlinkfixes branch May 6, 2021 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants