[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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.

Sign up for GitHub

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

Jump to bottom

Rotor output API #3242

Merged
merged 7 commits into from
Mar 2, 2021
Merged

Rotor output API #3242

merged 7 commits into from
Mar 2, 2021

Conversation

ahmed-elsaharti
Copy link
Contributor
@ahmed-elsaharti ahmed-elsaharti commented Dec 23, 2020 •
edited
Loading

Potentially helps with : #672 #2419

About

This PR adds an API that returns each rotor's speed, torque, and thrust.
This is potentially useful for the future development of energy/battery level simulation within AirSim using @sytelus 's method outlined here

The method used here is super crude and uses a global variable that gets updated with the rotor parameters with every updateRenderedState while waiting for the getRotorStates API call. This probably is not the most elegant way of doing this.
ie: Any testing and/or suggestions on how this could be improved are very welcome
Also, this is set up to work for quadrotors but scaling the struct up to take up any number of rotors is easy.

How Has This Been Tested?

This was tested with the Blocks environment on UE 4.24 with Simpleflight and PX4 SITL. No issues so far.

Screenshots:

On the python side here's the basic structure:
getRotorStates() (type = RotorStateAPI RotorStates) contains -> rotors_ rotors (type = RotorSTTVector RotorVector) contains -> rotors (type = list of rotors[0]-rotors[3] dicts) contains -> ['speed'], ['thrust'] and ['torque_scaler'] as floats

print(client.getRotorStates()):
image

print(client.getRotorStates().rotors_.rotor[0]):
image

zimmy87
zimmy87 suggested changes Feb 1, 2021
View reviewed changes
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.

Hi @ahmed-elsaharti, thank you for creating this PR! I have a few comments that describe an alternative to using the global variable in your code. Most of my other comments deal with naming conventions and style guidelines. Please feel free to ask any questions if you'd like extra clarification about any of my comments.

AirLib/include/common/CommonStructs.hpp Outdated Show resolved Hide resolved
AirLib/include/common/CommonStructs.hpp Outdated Show resolved Hide resolved
AirLib/include/vehicles/multirotor/api/MultirotorCommon.hpp Outdated Show resolved Hide resolved
AirLib/include/vehicles/multirotor/api/MultirotorRpcLibAdapators.hpp Outdated Show resolved Hide resolved
PythonClient/airsim/types.py Outdated Show resolved Hide resolved
AirLib/include/api/RpcLibAdapatorsBase.hpp Outdated Show resolved Hide resolved
AirLib/include/api/RpcLibAdapatorsBase.hpp Outdated Show resolved Hide resolved
AirLib/include/api/RpcLibAdapatorsBase.hpp Outdated Show resolved Hide resolved
AirLib/include/api/RpcLibAdapatorsBase.hpp Outdated Show resolved Hide resolved
AirLib/src/vehicles/multirotor/api/MultirotorApiBase.cpp Outdated Show resolved Hide resolved
@ahmed-elsaharti
Copy link
Contributor Author

Hi @zimmy87, Thank YOU for the EXTREMELY helpful comments!
I fixed all the naming issues, got rid of the global variable and generalized the size of 'rotors'.
Please let me know if any other modifications are needed.

@zimmy87
Copy link
Contributor
zimmy87 commented Feb 2, 2021

Hi @ahmed-elsaharti, 2 of the checks are failing for this PR due to a unrelated break in the Unity build from another PR. The fix for this build break was committed earlier today, can you rebase this PR to pick up the fix and rerun all the checks?

@ahmed-elsaharti
parent adb2b8d
c8436d1 
author Ahmed Elsaharti <ahmed.elsaharti@und.edu> 1608383085 -0600
committer Ahmed Elsaharti <ahmed.elsaharti@und.edu> 1612289167 -0600

Added API

Added timestamp

added getRotorStates, cleaned up

Got rid of global var

Generalized RotorVector
@ahmed-elsaharti ahmed-elsaharti marked this pull request as ready for review February 2, 2021 19:28
@ahmed-elsaharti
Copy link
Contributor Author

@zimmy87 Yup, that PR fixed the Unity build and the checks have passed 👍

zimmy87
zimmy87 suggested changes Feb 3, 2021
View reviewed changes
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.

This PR looks good for the most part, I just have 1 remaining comment.

AirLib/include/api/RpcLibAdapatorsBase.hpp Outdated Show resolved Hide resolved
rajat2004
rajat2004 reviewed Feb 4, 2021
View reviewed changes
AirLib/include/common/CommonStructs.hpp Outdated Show resolved Hide resolved
AirLib/include/vehicles/multirotor/api/MultirotorApiBase.hpp Outdated Show resolved Hide resolved
AirLib/include/api/RpcLibAdapatorsBase.hpp Outdated Show resolved Hide resolved
AirLib/include/api/RpcLibAdapatorsBase.hpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/Vehicles/Multirotor/MultirotorPawnSimApi.cpp Outdated Show resolved Hide resolved
AirLib/include/common/CommonStructs.hpp Outdated Show resolved Hide resolved
PythonClient/airsim/types.py Outdated Show resolved Hide resolved
zimmy87
zimmy87 suggested changes Feb 23, 2021
View reviewed changes
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.

I agree with most of @rajat2004's comments and think it would be prudent to include them before merging

AirLib/include/common/CommonStructs.hpp Outdated Show resolved Hide resolved
AirLib/include/vehicles/multirotor/api/MultirotorApiBase.hpp Outdated Show resolved Hide resolved
AirLib/include/common/CommonStructs.hpp Outdated Show resolved Hide resolved
PythonClient/airsim/types.py Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/Vehicles/Multirotor/MultirotorPawnSimApi.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/Vehicles/Multirotor/MultirotorPawnSimApi.cpp Outdated Show resolved Hide resolved
AirLib/include/api/RpcLibAdapatorsBase.hpp Outdated Show resolved Hide resolved
AirLib/include/api/RpcLibAdapatorsBase.hpp Outdated Show resolved Hide resolved
AirLib/src/vehicles/multirotor/api/MultirotorApiBase.cpp Outdated Show resolved Hide resolved
@ahmed-elsaharti
Copy link
Contributor Author

@rajat2004 @zimmy87 Thank you guys for the useful comments!
I can't for the life of me remember why I added RotorVector initially but I do agree, it is 100% redundant at this point.
I adjusted everything to:
image
rather than:
image
and added an initializer list to RotorParameters to keep things clean when populating/updating the rotor states.
One thing I know for sure is that I should stop looking at PR reviews after midnight. Everything made much more sense when I re-read the comments a few days later 😅😅

Anyway, the PR should be good to go now that the Github checks have run successfully. Let me know if any other modifications are needed.

rajat2004
rajat2004 reviewed Feb 26, 2021
View reviewed changes
Copy link
Contributor
@rajat2004 rajat2004 left a comment

Choose a reason for hiding this comment

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

Just some small nitpicks

AirLib/include/vehicles/multirotor/api/MultirotorApiBase.hpp Outdated Show resolved Hide resolved
AirLib/include/vehicles/multirotor/api/MultirotorRpcLibAdapators.hpp Outdated Show resolved Hide resolved
AirLib/include/common/CommonStructs.hpp Outdated Show resolved Hide resolved
AirLib/include/api/RpcLibAdapatorsBase.hpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/Vehicles/Multirotor/MultirotorPawnSimApi.cpp Outdated Show resolved Hide resolved
PythonClient/airsim/client.py Outdated Show resolved Hide resolved
zimmy87
zimmy87 approved these changes Mar 2, 2021
View reviewed changes
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.

This PR looks good to me and once the one remaining feedback concerning populating the rotor states is resolved, I think it will be ready for merging

@ahmed-elsaharti
Copy link
Contributor Author

@zimmy87 the last commit (4d8b3c9) should take care of these changes. Let me know if any other changes are needed 👍

@zimmy87
Copy link
Contributor
zimmy87 commented Mar 2, 2021

Thank you for the contribution @ahmed-elsaharti!

@zimmy87 zimmy87 merged commit 416de29 into microsoft:master Mar 2, 2021
@jonyMarino jonyMarino mentioned this pull request Jun 3, 2021
Closed
@condeLucas
Copy link

Hi @ahmed-elsaharti

Thank you for your contribution.

Using @sytelus 's method to have the energy/battery consumption, we have only to multiply Thrust * torque_scaler * speed for each rotor and sum then during the simulation time?

Do you know the units of these measurements?

Thank you so much for your attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajat2004 rajat2004 rajat2004 left review comments

@zimmy87 zimmy87 zimmy87 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ahmed-elsaharti @zimmy87 @condeLucas @rajat2004