Skip to content

Sensors from JSON and more #2#3502

Closed
evroon wants to merge 18 commits into
microsoft:masterfrom
evroon:feature-sensor-config
Closed

Sensors from JSON and more #2#3502
evroon wants to merge 18 commits into
microsoft:masterfrom
evroon:feature-sensor-config

Conversation

@evroon

@evroon evroon commented Mar 22, 2021

Copy link
Copy Markdown
Contributor

Duplicate of #2821, but now without merge conflicts.

About

Proposing a few changes here:
-Added ability to load parameters for JSON for Barometer, Distance Sensor, IMU, GPS, Magnetometer
-Small change to python types doc, fixes a change I proposed before with better datatypes (float vs quaternion) for distance sensor
-Small change to documentation on linux build, sometimes deleting the 'binaries' and 'intermediate' folders in your unreal project after rebuilding airsim can fix errors where unreal tries to open content that doesnt exist anymore and causes the 'try rebuilding from source' error
-small change to move on path, replaces the position PID to use the velocity PID when traveling at constant altitude, elimnates shake problems

TODO:
-did not update documentation to show the new fields available for settings...

I implemented most of the feedback of #2821. But the indentation of some files is still incorrect and there are irrelevant changes in the PR.

Comment thread AirLib/include/common/AirSimSettings.hpp Outdated
Comment thread AirLib/include/common/AirSimSettings.hpp Outdated
Comment thread AirLib/include/sensors/barometer/BarometerSimpleParams.hpp Outdated
@evroon

evroon commented Mar 26, 2021

Copy link
Copy Markdown
Contributor Author

@rajat2004 I implemented your feedback. I will write documentation soon.

real_T update_latency = 0.0f; //sec
real_T update_frequency = 50; //Hz
real_T startup_delay = 0; //sec
Vector3r position = VectorMath::nanVector();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

position is unused as well, can be removed. Same for IMU, GPS and Magnetometer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I removed them.

@evroon

evroon commented May 19, 2021

Copy link
Copy Markdown
Contributor Author

@lovettchris is it correct you added the functionality of this PR already with PR #3546? If I look at the merge conflicts that seems to be the case.

I mean commit 006d4e2 specifically. Because if it is already merged, this PR can be closed.

@lovettchris

Copy link
Copy Markdown
Contributor

Hi yes, I did that change. It looks like we both tackled the same problem, but implemented it in a slightly different way. Instead of populating all the fields in those structs in AirSimSettings.hpp like struct BarometerSetting, struct ImuSetting, struct GpsSetting, and so on, I instead decided to place the Json object inside the base class SensorSetting. This allows the json to be parsed instead inside each sensor actual *SimpleParams.hpp file. The reason I prefer this is because this way the "default values" for each sensor is kept in one place, namely the actual *SimpleParams.hpp files removing the duplication of default values from AirSimSettings.hpp.

@evroon

evroon commented May 19, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for the explanation. I indeed prefer your method as well, nice to see that it is now on master. I will close this PR then, because it has no further value. #2821 can also be closed, because it is an old version of this PR.

@evroon evroon closed this May 19, 2021
@lovettchris

Copy link
Copy Markdown
Contributor

sounds good, thanks.

@evroon
evroon deleted the feature-sensor-config branch June 28, 2021 09:10
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.

5 participants