add support for SITL PX4 Lockstep #2310
Conversation
merge upstream
Fix bugs in MavLinkComGenerator. Add setThreadName to aid in debugging.
…w PX4 SITL TCP support. TCP doesn't lose messages which is important for the new lockstep feature.
|
Could you provide high-level overview of the changes? It seems like huge changeset and its changing many public signatures and classes. Questions such as...
|
|
In addition to Shital's comments - I've some minor things to say initially - can you also please cleanup the git history - aka squash the merge commits and rebase on master. there are quite some whitespace in the diff as well. If it is standardizing old files, it's good. If not, let's remove that. |
|
Thanks for the review comments.
Ratnesh: Whitespace was a fix, one of my files had lots of TAB characters, so I untabbified the file. Can't we just squash merge when PR is accepted using this:
|
Add asset to make sure we are not on the PX4 publish thread when we block to receive more PX4 messages. Add ready state into to MultirotorState. update PX4 sitl docs accordingly.
…app is restarted. Add connection retry logic for serial port case. Check ground stability before allowing takeoff.
…ne to be armed or launched. This makes it easier to run a script with am/takeoff commands and have that script "just work" even when PX4 is in it's initialial unstable state.
… when accept is still pending.
sytelus
left a comment
sytelus
left a comment
There was a problem hiding this comment.
Overall looks pretty good. It would be great if you could add few comments as mentioned in the review. That would help me and anyone else in the future to understand code better.
AirLib/include/vehicles/multirotor/firmwares/arducopter/ArduCopterApi.hpp
Show resolved
Hide resolved
|
The naming of the settings fields seems strange, will comment if I have some suggestions. |
|
@madratman - changes looks good to me but I haven't tested it myself. If you are happy, please feel free to merge. |
|
I've tested this branch with ArduPilot, no functional changes as expected, backwards compatibility of settings is working fine. Haven't tested the PX4 side till now since did a new system installation recently and PX4 setup is left, will get to it soon. The renaming of the I was thinking of adding another field to the Settings, something like There were some problems with the altitude reported by AirSim which I noticed while testing, but it's not due to any changes in this PR, will try to look into it and maybe open an issue regarding that |
|
Rajat, thanks for testing ArduPilot. Regarding your question on renaming of sitl_ip_port to gcs_port, my reasoning is as follows. With PX4 in SITL mode one needs 2 sockets, a "hil" socket that sends HIL_SENSOR and other HIL_* messages, but this "hil" socket cannot be used to send flight commands. This socket is connected via the "udp_port" or "tcp_port". So as you can see naming the second socket "sitl_ip_port" the one that can sent flight controls, is a very confusing name. So I renamed it "gcs_port" which stands for "ground control station port", since it represents the port that can be used to "control" the drone. Renaming that "control_port" would be fine too. So anyway, I don't know if ArduPilot needs 2 sockets, if not, then it should use udp_port and ignore gcs_port. |
|
@lovettchris ArduPilot also needs 2 ports, there's actually only 1 socket being used, but there are 2 port numbers, one for the socket to bind to, earlier named The idea behind the renaming to |
|
@rajat2004, I see thanks. So ArduPilot should use udp_port to bootstrap and ignore gcs_port. I agree control_port is fine and gcs_port could be confused with qgc_port. So I'll rename it "control_port". |
|
@lovettchris Great, thanks a lot! |
…olPort to ControlPort in the settings.json and GroundControlIp ro ControlIp.
|
Ok, I pushed that fix, and here's a new build of neighborhood environment with the rename: |
|
Did the PX4 setup today, it installed Also, one more thing which I would like to ask, any thoughts on having a new release of AirSim? This PR would be a big reason to have one, and there are several other fixes and features, such as the ArduCopter support, already gone in and more in the current open PRs. It's been quite some time since the last release, especially for Linux, has been about a year since then. |
…ne PlayerStart location set to [160, -1500, 120]
|
@rajat2004, thanks for the info about conflicting clang-6 required by PX4. I agree a new release is in order after this PR goes in. I will bring it up with the team. |
|
I'm getting the following errors on connecting with PX4 only when running the blocks environment exported as package from UE4Editor. I don't get this error when I run the environment inside the UE4Editor using the play button. Any clue why this might be happening? Can anyone else please check if this error is reproducable? |
|
@saifullah3396, hmmm, would be great if you could help debug this some more, I can't repo that bug. The only thing that happens on the UdpClientPort immediately after "Ground control connected over UDP" is sending of heartbeats, which is a sendMessage call, but that happens deep down in this code which catches any exceptions from the port and rethrows with a different message, so it is strange that the UdpClientPort exception bubbled up as an unhandled exception: try {
port->write(message_buf, len);
}
catch (std::exception& e) {
throw std::runtime_error(Utils::stringf("MavLinkConnectionImpl: Error sending message on connection '%s', details: %s", name.c_str(), e.what()));
}And this is caught higher up and logged. It would be helpful to see the PX4 console output also. I do see this exception getting thrown occasionally on Windows, but it is always caught. |
|
@lovettchris I'll try to see if I can pinpoint the exception since it seems to be raised from a part of the code different from what you've shown. I'll also update the code with your latest commit and see if that resolves the issue. |
|
Hi @lovettchris, it is defined in PX4MultirotorApithat the tcp connectoin should be remade if broken and if px4 is restarted, however, I am not able to reconnect right afterwards. I have seen the configuration of the tcp sockets used in this case and I don't see the SO_REUSEADDR and SO_REUSEPORT options in there. Maybe adding those might fix this? I get this error on reconnection with px4. |
@lovettchris The error is begin raised when making a connection to logviewer in |
|
Mentioning it here since this is more active, could a repo owner look into restarting the Travis build? It can't test with UE, but still is useful for an initial test. There's also an open PR #2356 for reworking travis.yml and adding Windows build which is where things break more frequently |
|
Hi everyone, I start testing this solution and I can confirm as @saifullah3396 mentioned we need to add |
|
@rajat2004 , thanks for the stack trace. I can see why that is throwing an exception if log viewer is not there, but I can't see why that crashes the app, there should be a try/catch higher up the stack in MavLinkConnectionImpl::drainQueue. But for now I will change the default setting to: |
|
New Windows build of Neighborhood map with the latest bits. |
|
@lovettchris It is important to note that this error is only raised in packaged project. In the editor on pressing play and connecting with px4 I do get the upper level exception. Could there be some build flags in unreal engine that turns off exception handling while packaging the project? I used both the editor and the following script for packaging: |
|
@saifullah3396, I see, well that is interesting, there must be some sort of build difference then in how exceptions work in the packaged version... |
7 participants
This makes it possible to use PX4 SITL version 1.9.* and above with the lockstep feature. See Lockstep Simulation. Flying without lockstep on v1.8.* is still supported. Lockstep mode recommends a TCP connection which is new for AirSim. The new settings.json for this will be:
{ "SeeDocsAt": "https://github.com/Microsoft/AirSim/blob/master/docs/settings.md", "SettingsVersion": 1.2, "SimMode": "Multirotor", "Vehicles": { "PX4": { "VehicleType": "PX4Multirotor", "UseSerial": false, "UseTcp": true, "TcpPort": 4560, "ControlPort": 14580, "Parameters": { "NAV_RCL_ACT": 0, "NAV_DLL_ACT": 0, "LPE_LAT": 47.641468, "LPE_LON": -122.140165, "COM_OBL_ACT": 1 } } } }