A couple of small improvements to embedded c++ client

[jpwsutton]

migrated from Bugzilla #475751 status ASSIGNED severity enhancement in component MQTT-Embedded-C for 1.2 Reported in version 1.0 on platform Other Assigned to: Ian Craggs

On 2015-08-24 19:58:40 -0400, Mark Sonnentag wrote:

Just a couple of suggestions for improvements:

1.) this->var = var is bad style in c++. CDT even reports it as a warning (-wshadow) It is better to use a different name for the member variable (m_var, var_ or something like that).

2.) Casts: C-Style cast shouldn't be used in C++. C++ has its own casts. The advantage is that the C++ can be only used for particular tasks instead of the jack of all trades C cast. The intention of the cast is clear and the potentially most dangerous casts like reinterpret_cast and const_cast can be easily spotted in the source.

3.) Out-parameters: The out-parameters are passed as a reference to a variable. Imho it is better to pass variables that are modified as a pointer. This way it is clear for the caller that this variable is modified. References are fine for in-params (breaks the API)

4.) "Template" files for Network/Timer classes. Files with all necessary methods of the Network and Timer classes, empty bodies and some comments would help with porting the client.

5.) Currently the user of the mqtt client is supposed to connect to the mqtt broker server prior to telling the mqtt client to connect. This is my opinion bad design, because the mqtt client uses the network anyways. The result is that a connection and the mqtt client has to be injected in class X that uses the client, although the network connection is logically below the client. I think it would be be better if the mqtt client would establish the connection to the server at the beginning of the int MQTT::Client<Network, Timer, MAX_MQTT_PACKET_SIZE, b>::connect(MQTTPacket_connectData& options) method. The user of the client could implement a connect method in the Network class for this purpose. Same applies to disconnect. This breaks the current API, however the behavior could be controlled with a macro.

On 2015-08-25 07:16:32 -0400, Ian Craggs wrote:

1. do you mean referring to this-> at all? In the case that the parameter name is the same as the variable, I prefer to change the parameter name. So this is a couple of instances of

this->command_timeout_ms = command_timeout_ms;

2. The only hesitation I have with this is knowing which versions of C++ support this. I want the API to be as portable as possible, and embedded environments can have varying levels of C++ compiler. I think there is only one instance (enum QoS)?

3. Using references rather than pointers does have an effect on some APIs. References give a simpler and more native look and feel on the Arduini, for instance. I see the logic behind your suggestion, and would bear it in mind for a later time.

4. Definitely. This is a good idea.

5. I took this approach in the spirit of keeping the API as minimalist as I could (although the per subscription message handlers violate that spirit -- I added them due to popular demand). Thinking about it, I'm still more in favour of my current approach, although if we were changing the API substantially at some later point, I would bear it in mind.

On 2015-08-25 09:42:50 -0400, Mark Sonnentag wrote:

1.) Yes. There are more instances however like this->keepAliveInterval = options.keepAliveInterval; It is just not necessary and reduces a bit the amount of code that one has to read beside avoiding the warning by the compiler/cdt. Renaming the parameters works as well of course (var = aVar like the constructor of MessageData)

2.) Afaik there should not be a C++ compiler for any embedded device that does not know C++ casts. They have been part of C++98 Standard already. They are not a C++11 feature or anything like that.

There are quite few C style casts in the client source e.g.: if (MQTTDeserialize_publish((unsigned char_)&msg.dup, &intQoS, (unsigned char_)&msg.retained, (unsigned short_)&msg.id, &topicName, (unsigned char__)&msg.payload, (int_)&msg.payloadlen, readbuf, MAX_MQTT_PACKET_SIZE) != 1)

and even a ambigious cast (const cast): MQTTString topic = {(char*)topicFilter, {0, 0}};

You only know what happens here if you compare the datatypes. The C++ variant would be: MQTTString topic = {const_cast<char*>(topicFilter), {0, 0}};

Then everyone knows just by looking at the cast that const has been cast away of a char* and that it will never do anything else no matter what topicFilter is. If the code changes then C style cast variant could even become a reinterpret_cast and then you have a potential bug.

But it is good that you mention the (enum QoS) cast. This is potentially a bug. Casting an integer to an enum is undefined behavior in C++ if the integer value is not equal to at least one of the enum values. Someone could send a MQTT packet with a QOS value of 3.

3.) Yes I have seen that passing out parameters as a reference instead of pointers is quite common in an Arduino environment. A non API breaking alternative would be a clear documentation. Doxygen supports for example @param[in], @param[inout] and @param[out]

1. My idea was to add a macro to the network template file. If the macro is not defined then the couple of lines in the mqttclient connect and disconnect method are removed by the preprocessor. This way it technically breaks the API but it affects you only if you decide to use the feature otherwise the client code is the same as before and you can still decl/define connect/disconnect methods in the network however you like (see arduino/mbed samples).

Your approach of sticking with a minimal API is definitely the way to go for an embedded device. The main problem is just that the API is not consistent currently. The client can receive/send on its own by using the network stack however it cannot connect/disconnect. The same interface is used everytime.

If you want then I can push 1, 2, 4 and maybe 3(comment variant) to Gerrit after I have tested the changes against the test python server. The client could silently break with 2.