ThingSet / thingset-device-library

ThingSet library for resource-constrained devices written in C/C++
https://thingset.io/thingset-device-library/
Apache License 2.0
13 stars 6 forks source link

RFC: C implementation of ThingSet device library for Zephyr #9

Closed b0661 closed 3 years ago

b0661 commented 3 years ago

Introduction

It would be nice to have a C language interface ThingSet device library to support C only applications.

EDIT: Zephyr module infrastructure will become an extra PR. Commit removed here.

~~As a lot of IoT applications (at least mine !-) are using the Zepyhr RTOS a Zephyr module would ease the library use.~~

Problem description

There is (no known) C language interface ThingSet device library for C only applications. ~~Neither is there Zephyr module support in the ThingSet device library code base.~~

Proposed change

Base the ThingSet device library on a C implementation with a C++ interface shim. Provide it as a Zephyr module

Proposed change (Detailed)

Move the *.cpp implementation files to C implementations. Keep the functionality, naming, interface mostly the same as the current C++ implementation. Only the interface has to get an additional pointer to the ThingSet object it is working on.

Use the TS/ts_ namespace for the C implementation to avoid name clashes.

Replace printf calls by Zephyr style macros to provide for configurable environment specific logging and error reporting.

Keep the public interface of the C++ ThingSet class the same. Make the class a header only shim that uses the C implementation.

~~Create a zephyr sub-directory to contain all the Zephyr specific module configuration and additional code that is necessary for Zephyr only. Add some libc functions to this code that the Zephyr minimal libc does (currently) not provide to make the library usable without newlib.~~

Dependencies

Existing C++ applications that directly build from source have to use the C implementation files. There are no more *.cpp files.

Concerns and Unresolved Questions

None.

Alternatives

Alternative would be to use the C++ library implementation in C applications. As it is much easier to integrate C into C++ than vice versa this alternative was not regarded.

martinjaeger commented 3 years ago

Wow, thanks a lot for this contribution!

I had the conversion into a pure C library for Zephyr on my ToDo list since a long time already. Really cool.

I'm currently travelling for two weeks, so it might take a few days until I can provide a proper review.

Out of interest: For what sort of application are you using ThingSet and this library?

I was recently thinking about some small changes to the protocol to provide better support for MQTT integration (adding a topic part behind the # of the publication messages). And it was probably not a good idea to allow payload in the GET request in the binary mode, as this can't be translated to CoAP or HTTP. I'll let you know once I have properly documented those issues and ideas for improvement. Which mode are you using? The binary or the text mode?

b0661 commented 3 years ago

Out of interest: For what sort of application are you using ThingSet and this library?

It is a gateway/ sensor system with special but low cost sensors. In the past I used Firmata on serial to connect the sensors to the gateway. Now I am experimenting to use ThingSet also with the option to use different transports (SPI, CAN). Gateway and sensors are running on Zephyr. Gateway connection is planned to be WebSocket based.

Which mode are you using? The binary or the text mode?

My plan is text mode for service/ maintenance access. Binary for regular transmissions. Probably a text mode shell extension for Zephyr will be my next step.

b0661 commented 3 years ago

so that we can get the rest merged more quickly?

Do you prefer to have an extra commit for the changes addressing your comments (should make review easier) or shall I combine it all (without the Zephyr commit) into one?

martinjaeger commented 3 years ago

so that we can get the rest merged more quickly?

Do you prefer to have an extra commit for the changes addressing your comments (should make review easier) or shall I combine it all (without the Zephyr commit) into one?

You can either amend f7d00559b584b1b473dbfc556a468e98d618d0ae or add a separate commit addressing the comments. Both fine from my side. Squashing everything into one is not ideal, I think, as it's good to have commits without functional changes separately (exactly as you did).

martinjaeger commented 3 years ago

Ah, there we go... almost at the same time. I'll review the new comment immediately :)

martinjaeger commented 3 years ago

Alright, thanks a lot for the updates!

I tested it again and everything works fine, so I'm gonna merge it now.

Just realizing that Travis CI stopped working, so I'll look into that...