Refactoring - ASI CCD driver

[pawel-soja]

Hi!

Improving the readability of the driver.

indilib/indi#1389 is required for this PR.

Changes:

• Use INDI::Property.
• Use INDI::Timer, INDI::ElapsedTimer.
• Use SingleWorker for Exposure/StreamVideo.
• Add more ASI_ERROR_CODE checks.
• Unified nomenclature.
• Add string code error.
• Hotplug cameras - no driver restart needed.

[knro]

Thank you! This is the first time for hotplug implementation. I also see the worker thread implementation which could be used by other drivers as well! I will give this a go tonight.

[knro]

@pawel-soja Looks like there is some issue with the hotplug. One driver crashed, when I restarted (I have ASI120 and ASI178) and I get this:

2021-03-20T22:08:24: [ERROR] Error connecting to the CCD (ASI_ERROR_CAMERA_REMOVED).

I recycled the power and cannot use the camera anymore. Can you please check?

[pawel-soja]

@knro Can you describe exactly in what order you run indi_asi_ccd, connect / disconnect cameras?
I will throw in corrections tonight.

[knro]

I restarted the PI and it appears to be working now, will try to test more and see what happens.

[pawel-soja]

@knro
I flew through all the cases and found some bugs.
But I have a problem with one, I think there is a bug in KStar (3.4.3 Build: 2020-10-17T17:08:57Z).

Maybe as you saw I disabled the updateProperties function call.

indi-3rdparty/indi-asi/asi_ccd.cpp

Lines 386 to 395 in 99ec370

	ASICCD::~ASICCD()
	{
	if (isConnected())
	{
	Disconnect();
	// updateProperties();
	}
	}

I disabled it because it removes all controls from all cameras.
It seems that KStars does not consider the device name.

You can test it on a simple example.

#include <libindi/defaultdevice.h>
#include <libindi/indipropertylight.h>

class SimpleDevice : public INDI::DefaultDevice
{
    const char *name;
    INDI::PropertyLight LightLP {1};
public:
    SimpleDevice(const char *name)
        : name(name)
    {
        setDeviceName(name);
        LightLP[0].fill("LIGHT_NAME", "LIGHT_LABEL");
        LightLP.fill(getDeviceName(), "LIGHT_NAME_LP", "LIGHT_LABEL_LP", MAIN_CONTROL_TAB, IPS_OK);
    }

    bool updateProperties() override
    {
        DefaultDevice::updateProperties();
        if (isConnected())
        {
            //defineProperty(LightLP);
            IDDefLight(&LightLP, nullptr);
        }
        else
        {
            //deleteProperty(LightLP.getName());
            IDDelete(LightLP.getDeviceName(), LightLP.getName(), nullptr);
        }
        return true;
    }

    bool Connect() override { return true; }
    bool Disconnect() override { return true; }
    const char *getDefaultName() override { return name; }
};

static SimpleDevice simpleDevice1("Simple Device 1");
static SimpleDevice simpleDevice2("Simple Device 2");

Connect both devices, then click disconnect in one of them.

On the second tab - magic!

[pawel-soja]

KStars 3.5.3 works ;)

[knro]

Thanks for the confirmation! Let me know if you have anything else!

[pawel-soja]

I am testing how to remove the entire device from tabs.
I can see that deleteProperty(nullptr) should be able to do it.

bool DefaultDevice::deleteProperty(const char *propertyName)
{
    D_PTR(DefaultDevice);
    char errmsg[MAXRBUF];

    if (propertyName == nullptr)
    {
        //while(!pAll.empty()) delete bar.back(), bar.pop_back();
        IDDelete(getDeviceName(), nullptr, nullptr);
        return true;
    }

    // Keep dynamic properties in existing property list so they can be reused
    if (d->deleteDynamicProperties == false)
    {
        INDI::Property *prop = getProperty(propertyName);
        if (prop && prop->isDynamic())
        {
            IDDelete(getDeviceName(), propertyName, nullptr);
            return true;
        }
    }

    if (removeProperty(propertyName, errmsg) == 0)
    {
        IDDelete(getDeviceName(), propertyName, nullptr);
        return true;
    }
    else
        return false;
}

Unfortunately, all devices disappear (the 'INDI Control Panel' window closes).
Example:

#include <libindi/defaultdevice.h>
#include <libindi/indipropertylight.h>

class SimpleDevice : public INDI::DefaultDevice
{
    const char *name;
    INDI::PropertyLight LightLP {1};
public:
    SimpleDevice(const char *name)
        : name(name)
    {
        setDeviceName(name);
        LightLP[0].fill("LIGHT_NAME", "LIGHT_LABEL");
        LightLP.fill(getDeviceName(), "LIGHT_NAME_LP", "LIGHT_LABEL_LP", MAIN_CONTROL_TAB, IPS_OK);
    }

    bool updateProperties() override
    {
        DefaultDevice::updateProperties();
        if (isConnected())
        {
            //defineProperty(LightLP);
            IDDefLight(&LightLP, nullptr);
        }
        else
        {
            // try delete Simple Device X
            //deleteProperty(nullptr);
            IDDelete(getDeviceName(), nullptr, nullptr);
        }
        return true;
    }

    bool Connect() override { return true; }
    bool Disconnect() override { return true; }
    const char *getDefaultName() override { return name; }
};


static SimpleDevice simpleDevice1("Simple Device 1");
static SimpleDevice simpleDevice2("Simple Device 2");

[pawel-soja]

I found kstars/ekos/manager.cpp

void Manager::removeDevice(ISD::GDInterface * devInterface)
{
    //    switch (devInterface->getType())
    //    {
    //        case KSTARS_CCD:
    //            removeTabs();
    //            break;
    //        default:
    //            break;
    //    }

    if (alignProcess)
        alignProcess->removeDevice(devInterface);
    if (captureProcess)
        captureProcess->removeDevice(devInterface);
    if (focusProcess)
        focusProcess->removeDevice(devInterface);
    if (mountProcess)
        mountProcess->removeDevice(devInterface);
    if (guideProcess)
        guideProcess->removeDevice(devInterface);
    if (domeProcess)
        domeProcess->removeDevice(devInterface);
    if (weatherProcess)
        weatherProcess->removeDevice(devInterface);
    if (dustCapProcess)
    {
        dustCapProcess->removeDevice(devInterface);
        DarkLibrary::Instance()->removeDevice(devInterface);
    }

    appendLogText(i18n("%1 is offline.", devInterface->getDeviceName()));

    // #1 Remove from Generic Devices
    // Generic devices are ALL the devices we receive from INDI server
    // Whether Ekos cares about them (i.e. selected equipment) or extra devices we
    // do not care about
    for (auto &device : genericDevices)
    {
        if (device->getDeviceName() == devInterface->getDeviceName())
        {
            genericDevices.removeOne(device);
        }
    }

    // #2 Remove from Proxy device
    // Proxy devices are ALL the devices we specifically create in Ekos Manager to handle specific interfaces
    for (auto &device : proxyDevices)
    {
        if (device->getDeviceName() == devInterface->getDeviceName())
        {
            proxyDevices.removeOne(device);
            device->deleteLater();
        }
    }

    // #3 Remove from Ekos Managed Device
    // Managed devices are devices selected by the user in the device profile
    for (auto it = managedDevices.begin(); it != managedDevices.end();)
    {
        if (it.value()->getDeviceName() == devInterface->getDeviceName())
        {
            it = managedDevices.erase(it);
        }
        else
            ++it;
    }

    if (managedDevices.isEmpty())  // <====================================
    {
        cleanDevices();
        removeTabs();
    }
}

@knro Is the implementation at the end of the function with intention?
What's the point of that?

Maybe it should be something like:

    if (managedDevices.isEmpty() && genericDevices.isEmpty() && proxyDevices.isEmpty())
    {
        cleanDevices();
        removeTabs();
    }

[knro]

Nice detective work @pawel-soja. I will check why it was done in this way. There are lots of scenarios that requires testing (remote and local drivers, chained server...etc).

[pawel-soja]

Fantastic!

I would like to add something like this (indilib):

DefaultDevicePrivate::~DefaultDevicePrivate()
{
    const std::unique_lock<std::recursive_mutex> lock(DefaultDevicePrivate::devicesLock);
    devices.remove(this);
    IDDelete(deviceName.c_str(), nullptr, nullptr); // <==== added
}