Skip to content

Conversation

Greblys
Copy link

@Greblys Greblys commented Jul 27, 2016

I think this is a better approach to share same Timer and Network interfaces between different platforms. The actual structure definitions for Timer and Network objects is hidden from MQTTClient and it contains only void * pointers to these Timer and Network objects. The functions declarations which needs to be implemented by specific platform are in Timer.h and Network.h header files. Each platform implements them and MQTTClient uses it like this:

void *networkObjectPointer;
networkObjectPointer = createNetwork(); //platform creates a network object for you
connect(networkObjectPointer); //platform will connect you to the network, but you need to pass the network object to it. 
read(networkObjectPointer, buffer, timeout); 
destroyNetwork(networkObjectPointer); //destroy network object and free its memory 

Currently I applied these interfaces only to linux implementation. This approach simplifies compilation and no sed magic is needed in the build script. Have a look into build script for stdoutsub.c.

jpwsutton and others added 5 commits February 29, 2016 11:13
Signed-off-by: James Sutton <james.sutton@uk.ibm.com>
Updating CONTRIBUTING.md & README.md on master branch
…mentation

Signed-off-by: Lukas Greblikas <l.greblikas@cairnsolutions.com>
Signed-off-by: Lukas Greblikas <l.greblikas@cairnsolutions.com>
Copy link

@RostakaGmfun RostakaGmfun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!
I've added some comments suggesting improvements. I would be grateful if you prepare the PR for merging it because your changes are really useful to me.

Thanks in advance

#ifndef MQTT_CLIENT_C_NETWORK_H
#define MQTT_CLIENT_C_NETWORK_H

int mqttread(void *network, unsigned char* read_buffer, int, int);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to place forward declarations to Network structure here, to avoid using pointers to void everywhere.
Something like this:
typedef struct Network Network;

The same applies to Timer interface.

char TimerIsExpired(Timer* timer)
char TimerIsExpired(void *t)
{
Timer *timer = t;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about checking for NULL pointers everywhere?

typedef struct Network
{
int my_socket;
int (*mqttread) (struct Network*, unsigned char*, int, int);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative approach, you can keep the function pointers in the Network interface and implement them for each platform. Then, each platform provides the NetworkInit() function, which initializes these pointers to platform-specific functions. This way, you define the Network structure once in the common header and don't need any forward declarations.

@rafaeldelucena
Copy link
Contributor

Hi @Greblys this is a pretty cool PR!

Do you intend to end this PR? I can help if you're out of time.

Best regards

@icraggs
Copy link
Contributor

icraggs commented Jul 4, 2017

The IP check for this PR failed. I assume that a signature of "noreply@github.com" is an error? The IP check must pass for the PR to be merged.

@BruegelN
Copy link

BruegelN commented Aug 1, 2017

Hi there,

@icraggs thank for providing this embedded C implementation and thanks @Greblys for your improvements.
Is there any progress on this specific topic?
I do like the idea of using a timer and a network interface, including some suggestions made by @RostakaGmfun
Is there a way I can help regarding the current situation?

Best regards,
Nico

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.

6 participants