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

New Vehicle Heron USV for Webots #28251

Closed
wants to merge 0 commits into from

Conversation

harunkurtdev
Copy link

heron_ocean

By connecting heron usv to ardupilot on webots.
A new vehicle type has been added. This vehicle type is a marine vehicle and is well known in the market.

@rmackay9
Copy link
Contributor

Hi @harunkurtdev,

Thanks for this. I actually didn't realise that Webots was still usable. Good news that it is.

One thing is it would be best if the commits were squashed together somewhat and also if you peek at our commit history you'll see we always prefix commits with the subsystem (or folder) affected. This makes it easier for us to backport features.

@harunkurtdev
Copy link
Author

Hi @harunkurtdev,

Thanks for this. I actually didn't realise that Webots was still usable. Good news that it is.

One thing is it would be best if the commits were squashed together somewhat and also if you peek at our commit history you'll see we always prefix commits with the subsystem (or folder) affected. This makes it easier for us to backport features.

So sir, what should I do? Can you recommend me a guideline?

I look this contributing

@harunkurtdev
Copy link
Author

harunkurtdev commented Sep 28, 2024

Mr. @rmackay9, it is working, what should I do now? I did not add any file confusion that would prevent Ardupilot from working.
Just new vehicle and world for webots.

@peterbarker
Copy link
Contributor

Hi @harunkurtdev,
Thanks for this. I actually didn't realise that Webots was still usable. Good news that it is.
One thing is it would be best if the commits were squashed together somewhat and also if you peek at our commit history you'll see we always prefix commits with the subsystem (or folder) affected. This makes it easier for us to backport features.

So sir, what should I do? Can you recommend me a guideline?

I look this contributing

The information you're after is here: https://ardupilot.org/dev/docs/git-interactive-rebase.html

@@ -45,6 +48,7 @@ void WebotsPython::set_interface_ports(const char* address, const int port_in, c
// try to bind to a specific port so that if we restart ArduPilot
// Webots keeps sending us packets. Not strictly necessary but
// useful for debugging

Copy link
Contributor

Choose a reason for hiding this comment

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

Bad whitespace change

Comment on lines 57 to 69
_webots_address = address;

/*

if (is_wsl()) {
_webots_address = "0.0.0.0"; // Use 0.0.0.0 in WSL
} else {
_webots_address = address; // Use localhost otherwise
}
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed change?

@@ -128,6 +141,21 @@ void WebotsPython::recv_fdm(const struct sitl_input &input)

}

bool WebotsPython::is_wsl(){
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused?

Comment on lines 32 to 34



Copy link
Contributor

Choose a reason for hiding this comment

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

Bad whitespace change

@@ -67,6 +70,10 @@ class WebotsPython : public Aircraft {
double position_xyz[3];
};


/// @brief check for wsl env
bool is_wsl();
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused?

@@ -155,14 +155,13 @@ def _handle_sitl(self, sitl_address: str = "127.0.0.1", port: int = 9002):
Args:
port (int, optional): Port to listen for SITL on. Defaults to 9002.
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

bad whitespace change?

# create a local UDP socket server to listen for SITL
s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) # SOCK_STREAM
s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
s.bind(('0.0.0.0', port))

# wait for SITL to connect
print(f"Listening for ardupilot SITL (I{self._instance}) at 127.0.0.1:{port}")
print(f"Listening for ardupilot SITL (I{self._instance}) at {sitl_address}:{port}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we just bind 0.0.0.0?

I mean, the existing comment is wrong, but this one is also wrong.

I mean, less wrong :-)

@@ -241,12 +240,13 @@ def _handle_controls(self, command: tuple):
Args:
command (tuple): tuple of motor speeds 0.0-1.0 where -1.0 is unused
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

bad whitespace change

# get only the number of motors we have
command_motors = command[:len(self._motors)]
if -1 in command_motors:
print(f"Warning: SITL provided {command.index(-1)} motors "
f"but model specifies {len(self._motors)} (I{self._instance})")
command_motors = [v if v != -1 else 0.0 for v in command_motors]
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment as to why this is needed, please.

SERVO4_FUNCTION 0 # Disabled (no steering required)

AHRS_EKF_TYPE 10 # Use sim AHRS
#ARMING_CHECK 1045534 # No RC requirements
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove RC here? Simulation has it by default?

If this was copied from elsewhere that's fine.

@peterbarker
Copy link
Contributor

@harunkurtdev still waiting for some of my comments to be addressed

@harunkurtdev
Copy link
Author

harunkurtdev commented Nov 10, 2024

@harunkurtdev still waiting for some of my comments to be addressed

Mr. @peterbarker,

I removed the parts you took into consideration and the unused parts.

Which part do I need to edit? Could you please state it again?

Sir, Are these what you've been waiting for?

#28251 (comment)
and
#28251 (comment)

It just didn't work on WSL in Windows. I had added some to them but since it still didn't work and you told me to, I removed it.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Ping @harunkurtdev - there are many un-addressed review comments in here.

libraries/SITL/SIM_Webots_Python.cpp Outdated Show resolved Hide resolved
libraries/SITL/SIM_Webots_Python.cpp Outdated Show resolved Hide resolved
@harunkurtdev harunkurtdev deleted the pr-webots-usv branch November 13, 2024 07:28
@harunkurtdev harunkurtdev restored the pr-webots-usv branch November 13, 2024 07:28
@harunkurtdev harunkurtdev deleted the pr-webots-usv branch November 13, 2024 07:28
This was referenced Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants