set_root_directory() does not set the root directory (FTP)

[thomasteisberg]

The set_root_directory function is supposed to update the local root directory for the FTP service by updating _root_dir in MavlinkFtp.

I added LogWarn() << "_root_dir: " << _root_dir; to the _work_list() function and it still prints the default value of _root_dir (which is .) even after calling set_root_directory.

To reproduce: The easiest thing is to run the FTP server example (https://github.com/mavlink/MAVSDK/blob/main/examples/ftp_server/ftp_server.cpp) and try passing in different root directories. No matter what you pass in, the root directory will always be set to the current working directory (i.e. ., the default value of _root_dir).

Big thanks for @julianoes for helping me work through this and some other FTP issues.

I'm guessing this is going to be a simple fix, but I can't see the issue and Julian hasn't had time, so I'm making this issue in the hopes that someone else might see what's going wrong.

[thomasteisberg]

I think I figured out why this appears to be happening, but it's unclear to me what the behavior is supposed to be.

Every time a new system is connected (i.e. a mavlink thing with a unique system ID sends its first message), it gets detected and a new System instance is created. This new System instance in turn also creates a new MavlinkFtp instance.

In the way I was testing, it's very easy to call set_root_directory() on the wrong instance of MavlinkFtp. Usually, I would start up the ftp server (at which point it has only identified one system - the autopilot with sysid 0) and so I would get the System and set the root directory, which updates _root_dir on the first instance of MavlinkFtp.

Now I run mavproxy and the server discovers this and creates a new System corresponding to sysid 255 (or whatever I set in mavproxy). Creating this new System also creates a second MavlinkFtp instance, which still has _root_dir set to whatever the default is. Now I do ftp list or whatever from mavproxy and this second instance of MavlinkFtp handles the request, creating the appearance that my call to set_root_directory() did nothing.

As a proof of concept, this workaround appears to work:

while (true) {
    std::this_thread::sleep_for(std::chrono::seconds(1));

    for (auto system : mavsdk.systems()) { // Iterate through all available systems and call set_root_directory
        auto ftp_server = Ftp{system};
        ftp_server.set_root_directory("/home/ubuntu/ftp_test");
    }
}

In the context of acting as an FTP server, it doesn't make much sense (to me) to have the instance or any properties associated with a specific client system. The FTP server behavior should, I would think, be the same independent of which system is talking to it.

That said, the MavlinkFtp class is shared between both FTP client and FTP server purposes, and there is some logic to having client tasks associated with a specific connection.

One option would be to make _root_dir a static property of MavlinkFtp. This ensures that no matter which instances get created, there is only a single _root_dir. It also means that set_root_directory() can be made into a static function, which means that you can set a root directory even if no devices are yet attached. (Meaning that the companion computer can boot up before the autopilot and everything will still work as expected.)

With this modification, the ftp server example would look something like this:

    Mavsdk mavsdk;
    Mavsdk::Configuration configuration(Mavsdk::Configuration::UsageType::CompanionComputer);
    mavsdk.set_configuration(configuration);

    ConnectionResult connection_result = mavsdk.add_any_connection("serial:///dev/ttyS0:57600", ForwardingOption::ForwardingOn);
    if (connection_result != ConnectionResult::Success) {
        std::cerr << "Error setting up Mavlink FTP server.\n";
        return 1;
    }

    Ftp::set_root_directory("/home/ubuntu/ftp_test");

    while (true) {
        std::this_thread::sleep_for(std::chrono::seconds(1));
    }

I have a local branch where I've tried this approach. If the static _root_dir approach seems acceptable, I'd be happy to create a PR.

[thomasteisberg]

Just noticed @julianoes 's PR which would probably be the right solution to this eventually: #1733