Fix various inconsistencies, warnings

[rajat2004]

No description provided.

[madratman]

whitespace

[rajat2004]

It's a tab<->spaces problem in the entire file
Should I fix that as well?

[rajat2004]

Added commit for that, can be easily dropped if not needed

[rajat2004]

Since we're now moving ahead from 4.18, we'll need to update the docs to reflect that
Shouldn't we also update the compiler being used (currently clang-5)

[madratman]

Yes, I'll be making a pr for clang update

[rajat2004]

@madratman In the last commit, I've moved the frame types from the individual firmware params to MultiRotorParams, this now includes all the frame types used in PX4, so that the others can also use it, an example could be the Hexacopter model
Individual firmwares can override to setup their own specific ones, call the generic ones and then modify something if required, or have a different frame type such as in ArduCopterSolo

[rajat2004]

Another leftover, removing this does make the struct bigger, but I think better than earlier
The kArduCopterRotorControlsCount = 11 is the size of the packet received, 11 also doesn't make much sense, but is enough for larger frames, could be increased to 12 or something for those type of frames, but not changing this for compatibility

[rajat2004]

Another thing which I noticed, there are many APIs which I think are just for MultiRotors, but are defined in the VehicleClient class rather than the MultirotorClient class which can cause confusion
Most of the APIs below this - https://github.com/microsoft/AirSim/blob/master/PythonClient/airsim/client.py#L354

There are also many deprecated functions which can be removed when going for a new release

[madratman]

Hey @rajat2004. thanks for this
I ported your last commit in #2494. So please remove that
also, please move second and third last commit in a new PR.

Till, ArduCopterSolo: Tabs->Spaces looks good to me.

Hope there's no issue in getting the sensor data with the base class method we talked about. Will test out soon.

[rajat2004]

@madratman Done

[rajat2004]

I think using the new base class Sensor APIs can also help in another way, PX4, ArduPilot require some sensors by default such as IMU, GPS, etc.
But problems happen when sensors are specifically defined, and if they are missing, since then it all crashes. What could be done is an extra setting such as AddDefaultSensors can be added, which will automatically add the default sensors required, with some specific names, and the firmwares can just access those sensors
What do you think about this?

[madratman]

I think we already have default sensors you're talking about, along with a mechanism to instantiate them at json parse time?

In MultiRotorParamsFactory.hpp, we make a MultiRotorParams object called config.
In L44 of that file config->initialize(vehicle_setting) is called.
Hence, in MultirotorParams::initialize(), we see a call to addSensorsFromSettings(vehicle_setting);.

    void addSensorsFromSettings(const AirSimSettings::VehicleSetting* vehicle_setting)
    {
        // use sensors from vehicle settings; if empty list, use default sensors.
        // note that the vehicle settings completely override the default sensor "list";
        // there is no piecemeal add/remove/update per sensor.
        const std::map<std::string, std::unique_ptr<AirSimSettings::SensorSetting>>& sensor_settings
            = vehicle_setting->sensors.size() > 0 ? vehicle_setting->sensors : AirSimSettings::AirSimSettings::singleton().sensor_defaults;

        getSensorFactory()->createSensorsFromSettings(sensor_settings, sensors_, sensor_storage_);
    }

that sensor_defaults is of type std::map<std::string, std::unique_ptr<SensorSetting>> and is created in AirSimSettings::load() which calls AirSimSettings::loadDefaultSensorSettings() which calls AirSimSettings::createDefaultSensorSettings, which looks like:

    // creates default sensor list when none specified in json
    static void createDefaultSensorSettings(const std::string& simmode_name,
        std::map<std::string, std::unique_ptr<SensorSetting>>& sensors)
    {
        if (simmode_name == "Multirotor") {
            sensors["imu"] = createSensorSetting(SensorBase::SensorType::Imu, "imu", true);
            sensors["magnetometer"] = createSensorSetting(SensorBase::SensorType::Magnetometer, "magnetometer", true);
            sensors["gps"] = createSensorSetting(SensorBase::SensorType::Gps, "gps", true);
            sensors["barometer"] = createSensorSetting(SensorBase::SensorType::Barometer, "barometer", true);
        }
        else if (simmode_name == "Car") {
            sensors["gps"] = createSensorSetting(SensorBase::SensorType::Gps, "gps", true);
        }
        else {
            // no sensors added for other modes
        }
    }

My understanding is that these defaults are overridden in the case a user specifies custom sensors, and yes, in that case, the code will crash, if that's what you meant and that's what you are trying to prevent.

[madratman]

Hmm, if the last sentence of my verbose comment above is what you meant - then in MultirotorParams::initialize, an if-else check for vehicle type == PX4 | arducopter should be added. and default sensor list should always be loaded

[rajat2004]

@madratman Thanks for the detailed explanation, and yes, I did mean what you stated in the last sentence. I've had this happen to me as well, plus some issues are also there regarding this.
Adding a new feature (layer?, I don't know) like this could probably be done in the AirSimSettings.hpp itself, wherein it checks for the AddDefaultSensors field, and if set to true, then can add the default sensors
The previous behaviour won't be changed, so can still specifically add and remove sensors, have 0 sensors if they want, but it should give an extra layer of crash safety and easier usage, since if adding a Lidar, then just need to specify that, and leave others to AirSim to figure out

[madratman]

" Shouldn't we also update the compiler being used (currently clang-5)" -> #2500
test this for me?

[madratman]

"AirSimSettings.hpp itself, wherein it checks for the AddDefaultSensors field, and if set to true, then can add the default sensors" -> I think I saw it checking exactly for this in the file?

[madratman]

ah hang on, I have made a mistake in 2500. was porting from a private repo. Restoring...

[rajat2004]

"AirSimSettings.hpp itself, wherein it checks for the AddDefaultSensors field, and if set to true, then can add the default sensors" -> I think I saw it checking exactly for this in the file?

Yes, noticed that right now, had forgotten about it, thanks for pointing it out! Should have gone through the file properly before posting. Curiously, this info is not mentioned in the settings.md, it is present in sensors.md, but the sensors page is not linked from the main settings file.
Anyways, will test it out

" Shouldn't we also update the compiler being used (currently clang-5)" -> #2500

Yup, will do, thanks for the PR!

[madratman]

ah I ll add sensors.md in nav bar. A few other doc pages are not in the github.io site as well - docker, unity, and everything in wiki.
yeah, I'd suggest putting a breakpoint in simmode's/ multirotorpawnsimapi's initialize / construction methods to understand everything going on at construction time