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

asi_ccd/refactoring #266

Merged
merged 7 commits into from Dec 19, 2020
Merged

asi_ccd/refactoring #266

merged 7 commits into from Dec 19, 2020

Conversation

pawel-soja
Copy link
Contributor

More fixes for C++.
In short, using std containers.

Regards,
Pawel

@pawel-soja
Copy link
Contributor Author

I really like the ASICCD class. There are switches with dynamic quantity. This is the perfect place to experiment with generic widgets.

A snippet of asi_ccd.h

        /** Additional Properties to INDI::CCD */
        INDI::Numbers         CoolerNumber   {1};
        INDI::Switches        CoolerSwitches {2};

        INDI::Numbers         ControlNumbers;  // dynamic
        INDI::Switches        ControlSwitches; // dynamic

        INDI::Switches        VideoFormatSwitches; // dynamic

        uint8_t rememberVideoFormat = { 0 };
        ASI_IMG_TYPE currentVideoFormat;

        enum {
                BLINK_COUNT,
                BLINK_DURATION
        };

        INDI::Numbers         BlinkNumbers    {2};
        INDI::Numbers         ADCDepthNumbers {1};

        INDI::Texts           SDKVersionTexts {1};

Widgets could be implemented as described in the proposal Indi #1286

First draft.

#pragma once

#include "indiapi.h"

namespace INDI
{

template <typename T>
struct WidgetPropertyType;

template <>
struct WidgetPropertyType<ISwitch>: ISwitchVectorProperty
{
    using Type = ISwitchVectorProperty;
    void setWidget(std::vector<ISwitch> &widget)
    {
        nsp = widget.size(); // yep, each structure has a different field name.
        sp = widget.data(); // maybe "union" for new names and backward compatibility?
    }
};

template <>
struct WidgetPropertyType<INumber>: INumberVectorProperty
{
    using Type = INumberVectorProperty;
    void setWidget(std::vector<INumber> &widget)
    {
        nnp = widget.size(); // ...
        np = widget.data(); // ...
    }
};

template <>
struct WidgetPropertyType<IText>: ITextVectorProperty
{
    using Type = ITextVectorProperty;
    void setWidget(std::vector<IText> &widget)
    {
        ntp = widget.size(); // ...
        tp = widget.data(); // ...
    }
};
// ... //


template <typename T>
class Widgets: public std::vector<T>, public WidgetPropertyType<T>
{
public:
    using WidgetProperty = WidgetPropertyType<T>;
    using Widget = std::vector<T>;
    using Widget::Widget;

public:
    // For quick refactoring of current libraries.
    typename WidgetProperty::Type * operator&()
    {
        WidgetProperty::setWidget(*this);
        return this;
    }

    operator T*()
    {
        return std::vector<T>::data();
    }

public:
    // ... stuff ... //
};

using Switches = Widgets<ISwitch>;
using Numbers  = Widgets<INumber>;
using Texts    = Widgets<IText>;
// ... //

}

As you know, to make one button you have to declare 2 structures and handle a lot of functions.
I propose to encapsulate everything in generic classes. Additional built-in event handling will keep the driver code typing to a minimum.

What do you think?

@knro
Copy link
Collaborator

knro commented Dec 19, 2020

Looks good! Have you actually tested this for ZWO ASI driver using these classes fully? Newer drivers can make use of this (needs good documentation and HOWTO as well for developers) while we maintain existing drivers using the structs directly.

@knro knro merged commit 591a49c into indilib:master Dec 19, 2020
@xthestreams
Copy link

I have just complied this code and tested and it seems to be causing crashes on my system. (1.8.8 binaries from apt-get plus home re-complied drivers while I have been trying to debug the Paramount driver)

@pawel-soja
Copy link
Contributor Author

pawel-soja commented Jan 13, 2021

Thanks. Will test the changes again.
When exactly does the error occur?

@pawel-soja
Copy link
Contributor Author

@xthestreams
I tested it on a ZWO camera and can't reproduce the problem.
Which commit are you compiling?
What camera do you have connected?
Crash when starting indi_asi_ccd?
Please provide more details.

@xthestreams
Copy link

It's crashing on startup (the "play" button in Ekos). In answer to which commit, I am not 100% sure - I did a pull from git per the developers handbook and then complied.
log_10-02-51.txt

Attached the log.

@xthestreams
Copy link

It gets more interesting! I've noticed that the NexDome and FLI drivers are also both crashing on startup but the Paramount driver isn't. Whatever it causing the problem, I don't think it's these changes!

@xthestreams
Copy link

seems I'm not the only one - this "looping" behaviour is what I am seeing.

https://indilib.org/forum/ekos/8619-eqmod-crash-with-latest-stable-kstars.html#65903

@xthestreams
Copy link

By the way, the above looks amazing, I've been struggling to get my head around driver development and tweaking, this refactoring looks like it will help!

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.

None yet

3 participants