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

Mode Select Cluster Implementation has too many unnecessary abstraction #11163

Closed
du48s03 opened this issue Oct 28, 2021 · 0 comments · Fixed by #11359
Closed

Mode Select Cluster Implementation has too many unnecessary abstraction #11163

du48s03 opened this issue Oct 28, 2021 · 0 comments · Fixed by #11359

Comments

@du48s03
Copy link
Contributor

du48s03 commented Oct 28, 2021

Problem

The current implementation of Mode Select Cluster has some problems:

  • Over-designed interfaces and iterators that have a lot of virtuals on them, that are not necessary. This results in 24K increase in flash alone, without any of the application bits at all involved here. That is concerning.
  • Went through a lot of work to design a SupportedModesManager interface, yet the cluster impl only permits a specific concrete realization of that interface, which defeats the point of that interface.

Proposed Solution

This can be done much simpler: creation of a fairly simple 'data provider' interface that hides away the actual list of modes, as well as the methods to interact with that data feels like the appropriate solution here. This would hide away the storage specifics of that data (letting the application decide it), making it more flexible, as well as reducing the complexity of the overall solution.

A suggested provider interface (that would be registered with the cluster impl) could look like this:

class ModeDataProvider {
public:
    class ModeChangeListener {
    public:
        enum ModeOperation {
            kModeAdded = 0,
            kModeDeleted = 1,
            kModeModified = 2
        };

        virtual void OnModeChange(uint16_t modeIndex, ModeOperation modeOperation) = 0;
    };

    virtual CHIP_ERROR GetNumSupportedModes() = 0;

    virtual CHIP_ERROR GetMode(uint16_t modeIndex, Structs::ModeOptionsStruct::Type &mode, EndpointId &associatedEndpointId) = 0;
    virtual CHIP_ERROR SetMode(uint16_t modeIndex, Structs::ModeOptionStruct::Type &mode) = 0;

    virtual CHIP_ERROR RegisterModeChangeListener(ModeChangeListener *modeChangeListener) = 0;
};

This approximates very closely existing provider interfaces (like GroupDataProvider).

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 a pull request may close this issue.

1 participant