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

Timer and Network interfaces #43

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

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.

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.

{
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 "[email protected]" 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