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

Polymorphism and class inheritance #634

Closed
glmnet opened this issue Mar 12, 2019 · 10 comments
Closed

Polymorphism and class inheritance #634

glmnet opened this issue Mar 12, 2019 · 10 comments

Comments

@glmnet
Copy link

glmnet commented Mar 12, 2019

Hi, I see many classes which are AC related they have methods like void setTemp(const uint8_t temp);
However they are all plain classes which do not inherit from anything
Is there any reason why there is no BaseAC which some virtual methods for setTemp and other common methods to all ac?
I guess code from like the MQTTServer would beneficiate from this if instead of receiving a raw code (which has to be magically calculated outside the library) could instead receive the AC settings (temp, fan speed, etc) and then use this class + polymorphism to calculate the code (a feature already implemented)

@devyte
Copy link

devyte commented Mar 12, 2019

@glmnet
Hi, I'm not a developer of this repo's code, or even a user yet, but I am a maintainer of the Arduino core repo, and I've been keeping an eye on this repo.
I can tell you that object inheritance polymorphism is particularly ill-suited for small platforms such as the ESP, and my experience with the Arduino core, where inheritance polymorphism is widely used, has shown exactly that.
Each object instance has a vtable, and each vtable has one entry per virtual method. The more virtual methods you use, and the more objects you have, the more memory overhead at runtime. At our core repo we had one user report over 6KB of memory for the vtable entries in his app. That is beyond significant. There are many users who would give anything for an extra 6KB RAM.
In fact, to reduce the impact, we had to make the vtables configurable, and right now by default they reside on flash. Of course, that means that restrictions to objects with vtables apply, e.g.: you can't use them from ISRs.
There are other ways to address what you're seeking, such as template objects and functions, and common code refactoring, which allow for zero-overhead for both memory and execution. Usually the price to pay is a slight increase in binary size, but that can be mitigated if common code is factored out.

To be very clear: I strongly suggest not to propagate polymorphism in new code. Personally, I am happy that this repo has stayed away from it.

@glmnet
Copy link
Author

glmnet commented Mar 12, 2019

Ok, thanks for the explanation. I suspect some of that might be an issue.

@glmnet glmnet closed this as completed Mar 12, 2019
@glmnet glmnet reopened this Mar 12, 2019
@glmnet
Copy link
Author

glmnet commented Mar 12, 2019

@devyte so is this vtable using memory only when objects are instantiated? (I'm not a C++ expert, more C# guy here) but in any case one should create an specific instance of an AC type only when needed, and then discard it, this should use heap memory that should be freed as soon as code is sent.


BaseAC* ac;

switch (acModel) {
	case 'Coolix': 
		ac = new CoolixAc();
		break;
	case 'Whirlpool': 
		ac = new WhirlpoolAc();
		break;
}

ac->Send(onOffState, modeCoolHeat, temp, fanSpeed);

delete(ac);

Am I missing something here? Could you elaborate on a better approach to this problem?
Thank you.

@devyte
Copy link

devyte commented Mar 12, 2019

Yes, and that illustrates the case even more so. Polymorphism is appropriate when you have a bunch of different objects, have instances of them, and you need to treat all instances in a similar way. Classic example: 3D shapes where each shape knows how to render itself. You build a container with polygons, rectangles, squares, circles, whatever, and then you treat them all the same for transformations, drawing, etc.
In this case, you are likely using one specific type, so why
-allocate dynamically
-incur in virtual overhead at all
?

Taking your own code snippet: consider a template object that implements common behavior, let's call it BaseAC, and specific classes that implement individual things specific to each case, let's call those CoolixAc, WhirlpoolAc, etc. You could then do something like this:

switch (acModel) {
	case 'Coolix': 
                        sendAc<CoolixAc>(onOffState, modeCoolHeat, temp, fanSpeed);
		break;
	case 'Whirlpool': 
                        sendAc<WhirlpoolAc>(onOffState, modeCoolHeat, temp, fanSpeed);
		break;
}

template <typename AcType>
void sendAc(bool onOffState, tempMode modeCoolHeat, int temp, int fanSpeed) //instead of specific args you could have a template pack in generic manner, and forward that pack to the ac constructor. This allows to have different Ac objects with completely different arguments.
{
        BaseAc<AcType> AC(onOffState, modeCoolHeat, temp, fanSpeed);
        AC.send();
}

struct CoolixAc
{
        CoolixAc(bool onOffState, tempMode modeCoolHeat, int temp, int fanSpeed);
        ....
        std::vector<uint8_t> buildSendCommand();
        setupIRParams getIRSetup() {return setupIRParams(someparamshere);}

        //data members here, onOffState, tempMode, whatever           
};

class WhirlpoolAc
{
        WhirlpoolAc(bool onOffState, tempMode modeCoolHeat, int temp, int fanSpeed); 
        ....
        std::vector<uint8_t> buildSendCommand();
        setupIRParams getIRSetup() {return setupIRParams(someotherparamshere);}     

        //data members here, onOffState, tempMode, whatever           
};

template <typename AcType>
class BaseAc
{
public:
        BaseAc(bool onOffState, tempMode modeCoolHeat, int temp, int fanSpeed) 
           : ac(onOffState, tempMode, modeCoolHeat, temp, fanSpeed);

        void send() 
        {
           //somecommonlogicthatmakesuseoftheacmemberobject...
           auto params = _ac.getIRSetup();
           doIRSetup(params);
           auto command = _ac.buildSendCommand();
           doSendCommand(command);
        }
protected:
        AcType _ac;
};

Or you could do this, which is simpler on the template side, but can have more duplicity in the instantiation side:

switch (acModel) {
	case 'Coolix': 
                        BaseAc<CoolixAc> ac(onOffState, modeCoolHeat, temp, fanSpeed);
                        ac.send();
		break;
	case 'Whirlpool': 
                        BaseAc<WhirlpoolAc> ac(onOffState, modeCoolHeat, temp, fanSpeed);
                        ac.send();
		break;
}

struct CoolixAc
{
        CoolixAc(bool onOffState, tempMode modeCoolHeat, int temp, int fanSpeed);
        ....
        std::vector<uint8_t> buildSendCommand();
        setupIRParams getIRSetup() {return setupIRParams(someparamshere);}     

        //data members here, onOffState, tempMode, whatever           
};

struct WhirlpoolAc
{
        WhirlpoolAc(bool onOffState, tempMode modeCoolHeat, int temp, int fanSpeed); 
        ....
        std::vector<uint8_t> buildSendCommand();
        setupIRParams getIRSetup() {return setupIRParams(someotherparamshere);}     

        //data members here, onOffState, tempMode, whatever           
};

template <typename AcType>
class BaseAc
{
public:
        BaseAc(bool onOffState, tempMode modeCoolHeat, int temp, int fanSpeed) 
           : ac(onOffState, tempMode, modeCoolHeat, temp, fanSpeed);

        void send() 
        {
           //somecommonlogicthatmakesuseoftheacmemberobject...
           auto params = _ac.getIRSetup();
           doIRSetup(params);
           auto command = _ac.buildSendCommand();
           doSendCommand(command);
        }
protected:
        AcType _ac;
};

The BaseAc object would implement common logic, e.g. (I'm guessing here, I don't know how the code in the repo works yet):

  • ask the template object for specific setup params for the IR sender
  • do the setup
  • create some buffer
  • ask the template object for a specific command sequence and put it in the buffer
  • send the sequence in the buffer through the IR

I used std::vector<uint8_t> for the command sequence, but it could be something else, like std::array, or even a smart pointer with a C-style array.

Disclaimer: the above is off the very top of my head, and is meant as a very general example on template usage. I don't know how well it applies to the specifics of the usage of the code here, given that I'm not a user of this code yet. There could be other approaches to the template object design. However, I think it should illustrate one possibility for template use.

@glmnet
Copy link
Author

glmnet commented Mar 12, 2019

Ok thanks for all the information.
I'm closing this however the way this is implemented I have two options:

  1. Use my Controller (OpenHAB) and calculate the AC code there, send the raw calculated code via mqtt and let MQTTServer send it to the AC.
  2. Send Mode, OnOff, temp, etc via mqtt in a comma separated string and use already implemented classes in IRRemote, hack the MQTTServer example to parse this and create a switch which will interpret this to calculate the code (plus CRC, etc) and finally send the code to the IR LED.
    I am pretty sure I'll go by 2, as I have 3 kind of different codes to generate. However I don't know how to share the code.

@glmnet glmnet closed this as completed Mar 12, 2019
@sheppy99
Copy link

sheppy99 commented Mar 12, 2019

I have intermittently working OpenHAB code for option one that you’re welcome to, albeit for sending the Daikin2 protocol via IRMQTTServer. Doing further work shortly today to try and work out why it’s intermittent and I won’t be able to post it until later tonight or tomorrow (GMT + 13)
It creates a long protocol string including current time, all parameters and 2 checksums before sending it over MQTT using the publish command via the MQTT 1 action bundle

@crankyoldgit
Copy link
Owner

At our core repo we had one user report over 6KB of memory for the vtable entries in his app. That is beyond significant. There are many users who would give anything for an extra 6KB RAM.

@devyte Wow. I had no idea how expensive it was, but I did figure it had some cost. When I develop a solution for a more universal model for handling A/Cs I'll definitely re-look up this issue.

Thanks for the awesome detail/response btw. I look forward to your future feedback/input if/when you use the library. ;-)

@jimmys01
Copy link
Contributor

jimmys01 commented Mar 13, 2019

@glmnet

1. Use my Controller (OpenHAB) and calculate the AC code there, send the raw calculated code via mqtt and let MQTTServer send it to the AC.

I am very interested to your solution on this. I use .map files of this short.

OFF=IRSEND,RAW,FV025258L2K8KA48H8H2L8L24ALAK8H2403VO0H8HA58L252H24A48LA58H2LAL248H,38,480,486
COLD22c=IRSEND,RAW,VS04A4AHA58H8H8H248KALAH25AHAH24803VG0H8HA58L2525248H2HALA48LA5A48H,38,506,436
HOT22c=IRSEND,RAW,3VO08K8L2KAH2H2H248H8LAL248HA5AK8G07VG0H8HA58L2525248H2HALA48H2KAL8H,38,486,436
DRY=IRSEND,RAW,1VO04A4AHA58H8L8H248H2LAH252KAKA4803VG0H8HA58L252L248H24ALA48KAHAH8H,38,506,438

OFF=IRSEND,LG,88C0051
COLD22c=IRSEND,LG,8808721
HOT22c=IRSEND,LG,880C905
DRY=IRSEND,LG,8809946

@sheppy99
Copy link

My code is a whole lot more complicated as I construct the full hex string from scratch using a combination of fixed variables, user settings, the current time and also calculate a checksum. My code will likely create more questions than answers. In your case I’d just use a simple rule to build a string and then send that using a publish command. I’ll see if I can knock something together tomorrow to give you a clue, although this may be better posted in the OpenHAB community?

@sheppy99
Copy link

sheppy99 commented Mar 14, 2019

I can't test this, but it should work. I'm assuming you have a string item called "HeatpumpSetting" which is whatever mode you want it to select. I've also left in a couple of log statements which can be commented out once you know its working. It uses V1 versions of the MQTT binding and MQTT Actions
This goes in the .rules file

`

 var irToBeSent = ""
 rule "Send IR" 
 when Item HeatpumpSetting changed then
  logInfo("test", "received command is " +HeatpumpSetting.state)
   switch (HeatpumpSetting.state)	{ 
    case "OFF"      : irToBeSent = "IRSEND,LG,88C0051"
    case "COLD22c"  : irToBeSent = "IRSEND,LG,8808721"
    case "HOT22c"   : irToBeSent = "IRSEND,LG,880C905"
    case "DRY"	    : irToBeSent = "IRSEND,LG,8809946"
    }
logInfo("test", "IR to be sent "+irToBeSent)
publish("mosquitto","ir_server",irToBeSent) // change "mosquitto" to whatever your broker is called in mqtt.cfg, change "ir_server" to the topic your sending device is listening on
end

`

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

No branches or pull requests

5 participants