aws / aws-iot-device-sdk-embedded-C

SDK for connecting to AWS IoT from a device using embedded C.
MIT License
978 stars 625 forks source link

Should timer_linux.h be named timer_platform.h? #5

Closed projectgus closed 8 years ago

projectgus commented 8 years ago

I'm starting a port of this SDK to a new RTOS-based platform, and I noticed that the platform-agnostic timer.h includes timer_linux.h:

https://github.com/aws/aws-iot-device-sdk-embedded-C/blob/master/aws_iot_src/protocol/mqtt/aws_iot_embedded_client_wrapper/timer_interface.h#L31

As per the comment, this timer_linux.h header contains platform-specific declarations for the Timer structure.

Would a better name for this platform-specific header be timer_platform.h?

projectgus commented 8 years ago

As a follow up, I noticed that timer_interface.h treats "struct timer" as an opaque pointer. Is it necessary to include that header from the timer_interface.h at all, isn't a better design to keep it internal only to the timer implementation?

EDIT: I realised this would require InitTimer() to allocate the timer data structure internally, so I retract this second comment. Sorry for the noise!

bhadrip commented 8 years ago

Hi @projectgus ,

Yes it should be timer_platform.h, in this case it was linux platform. If it was some other platform say FreeRTOS, then it can be timer_freeRTOS.h.

Ideally we could have left it like

// Add the platform specific timer includes to define the Timer struct
#include " "

The linux file was left in as an example from our linux port.

Thanks for porting the SDK to your RTOS platform. Would you be able to provide more information about your port (what specific RTOS, target hardware)? We are always seeking input such as this to help improve the SDK.

Bhadri

projectgus commented 8 years ago

Thanks for the reply @bhadrip. Maybe I'm missing something, but why have people edit the platform-neutral file at all? Why not this?

In timer_interface.h (platform-neutral):

#include <timer_platform.h>

Then you have a platform_linux/common/timer_platform.h (same directory where timer_linux.h is currently).

If I add a FreeRTOS port for example, then I create platform_freeRTOS/common/timer_platform.h file.

And the compiler include paths are either -Iplatform_linux/common/ or -Iplatform_freeRTOS/common/, depending on what platform you're compiling for. (These include paths are already passed to the compiler.)

That way timer_interface.h really is platform neutral, noone needs to edit it for their platform.

Thanks for porting the SDK to your RTOS platform. Would you be able to provide more information about your port (what specific RTOS, target hardware)? We are always seeking input such as this to help improve the SDK.

I'm a maintainer for esp-open-rtos, which is a community-developed FreeRTOS port for the esp8266 microcontroller.

We have mbedTLS ported already, and there's a Paho MQTT client port out there, so it shouldn't be too hard to bring up the AWS IoT SDK as well. I'll let you know how we go.

bhadrip commented 8 years ago

Thats a great suggestion! It makes sense to keep the timer_interface.h platform neutral and handle it in makefile. We will discuss this solution internally.

The MQTT client distributed with the SDK is a modified version of Eclipse Paho Embedded C. Also, this file is the way we have configured mbedTLS.

Please keep us posted on how the port is coming up.

Thanks

vstefanyuk commented 8 years ago

@projectgus

Hello!

Any news about porting to esp8266?

Have really high hopes on it :)

Thank you very much for your work!

ramyogi commented 8 years ago

Eagerly waiting for ESP8266.

crivano commented 8 years ago

+1 for AWS IoT + NodeMCU (Lua)

jtHunter commented 8 years ago

+1 for using on esp8266

chaurah commented 8 years ago

Hi @projectgus, v2.x of the SDK finally includes the change to timer_platform.h as the new file name. Please take a look and let us know if you have any further suggestions for improvement.

Rahul