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

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

Closed
projectgus opened this issue Oct 27, 2015 · 9 comments
Closed

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

projectgus opened this issue Oct 27, 2015 · 9 comments

Comments

@projectgus
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

bhadrip commented Oct 27, 2015

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
Copy link
Contributor Author

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
Copy link
Contributor

bhadrip commented Oct 28, 2015

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
Copy link

@projectgus

Hello!

Any news about porting to esp8266?

Have really high hopes on it :)

Thank you very much for your work!

anujdeshpande pushed a commit to ButterBeacon/hardware that referenced this issue Jan 30, 2016
Adds CC32XX device support - http://www.ti.com/product/CC3200

Adds a top-level README_CC3200.md with setup instructions.

Adds platform_tirtos directories to each of the three sample apps
containing sources and build scripts for the CC3200 LaunchPad.

Note the oddity of adding a timer_linux.h file for TI-RTOS
platforms(!).  It's a misnomer, but doing so enabled _not_ requiring a
change to the common
aws_iot_src/protocol/mqtt/aws_iot_embedded_client_wrapper/timer_interface.h
source file.  Eventually, that common file may be changed to use a
better, generic name like timer_platform.h, as discussed here:
aws/aws-iot-device-sdk-embedded-C#5

Signed-off-by: Chris Ring <[email protected]>
@ramyogi
Copy link

ramyogi commented Apr 17, 2016

Eagerly waiting for ESP8266.

@crivano
Copy link

crivano commented Apr 24, 2016

+1 for AWS IoT + NodeMCU (Lua)

@jtHunter
Copy link

jtHunter commented Apr 29, 2016

+1 for using on esp8266

@chaurah
Copy link
Contributor

chaurah commented May 9, 2016

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

@chaurah chaurah closed this as completed May 9, 2016
aggarw13 added a commit that referenced this issue Nov 9, 2019
* Refactor code to break common callback type to API-specific callback types
* Update parser to de-serialize rejected response data and forward to callback for handling
* Add tests for rejected response parsing logic
* Update OnboardDevice API MQTT topic strings
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

7 participants