Changed platform interface to improve multiplatform support

[tilman1]

Dear Bob
please review and see whether it fits your design idea. The arduino-specific platform file I changed, but I could not load it into the arduino IDE in way that all the depend files where also loaded. Hence the compilation failed.

Cheers
Tilman

[bobveringa]

The compilation fails because #include <cstddef> is not available on Arduino platforms.

As for the changes, I like the idea, but I am not certain that I like the implementation. I feel like it changes some pretty fundamental things in the architecture. It did bring me onto some new ideas, but unfortunately, I am busy with work at this moment, and I don't have time to work on this driver for a little while.

I'll certainly use some of the ideas you have added here (and credit you for your work, of course) while working on the new implementation.

My main issue is adding (essentially) irrelevant fields to the device struct. Feels like there should be a more elegant solution, that separates the responsibility and concerns better.

[tilman1]

My main issue is adding (essentially) irrelevant fields to the device struct. Feels like there should be a more elegant solution, that >separates the responsibility and concerns better.

That is indeed not very elegant. The intention was to show that it in principle, it can be done like that.
The knowledge about the structure of hscdtd_device_t is needed in platform_*.cpp and in hscdtd008a_driver.cpp. One could include a platform specific header file defining hscdtd_device_t into both of these files. This would give each platform an individual device struct. Only needed fields would be available. If this is more elegant, I am not sure though.

[bobveringa]

This did bring me onto some ideas. I might have some time this weekend to give this a go.

[bobveringa]

I tried some things out and this is what I've come up with, this should (hopefully) also easily expand to other platforms. The idea of passing the hscdtd_device_t struct to the devices is a good idea, but storing the information in it seems cumbersome.

Instead, the hscdtd_device_t struct gets a new field tansport_cookie. Transport cookies consist of multiple parts, a generic part and a platform-specific part.

typedef struct {
    uint8_t addr;
    platform_cookie_t platform;

} hscdtd_transport_cookie_t;

It contains the address of the chip, as that is something that will be shared between all platforms (probably). It also contains a platform cookie. Platform cookies are platform specific and are defined in a new file platform_cookies.h They cannot be added to the platform.cpp files because that gives errors. And platform_cookies.h will look something like this.

typedef struct platform_cookie platform_cookie_t;


#ifdef ARDUINO
struct platform_cookie {
    
};
#endif //ARDUINO

#ifdef RPI
struct platform_cookie {
    char* device;
    int8_t fd;
};
#endif //RPI

Using this method, if there is ever a platform that requires 40 fields, it can just be added to the platform_cookie. And all the other elements stay clean.

@tilman1 What are your toughts on this?

[tilman1]

Dear Bob

I fail to see the problem that is to be solved by introducing an additional platform specific platform_cookie_t.
In my opinion, the same can also be achieved defining a platform specific hscdtd_device_t struct while all functions using it, get it as (generic) pointer:

#ifdef ARDUINO
struct hscdtd_device_t  {
 uint8_t addr;   
};
#endif //ARDUINO

#ifdef RPI
struct hscdtd_device_t  {
    uint8_t addr;
    char* device;
    int8_t fd;
};
#endif //ARDUINO

void open(struct hscdtd_device_t* device);
void close(struct hscdtd_device_t* device);
...

---- program ---

struct struct hscdtd_device_t* device;

open (&device);

--- end program ---
If it is the intention to have a common struct for all platform, such as the i2c address field that will be used in all platforms, one could implement an inheritance mechanism in C (that is not an object oriented language) using casting. This adds complexity and again, I do not think this additional complexity is necessary and the first approach will suffice.

struct hscdtd_device_t {
field a;
field b;    
};

#ifdef ARDUINO
struct hscdtd_arduino_device_t {
field a;
field b;    
field c; 
};
#endif //ARDUINO

#ifdef RPI
struct hscdtd_rpi_device_t {
    field a;
    field b;   
    char* device;
    int8_t fd;
};
#endif //RPI
void open(struct hscdtd_device_t* device);
void close(struct hscdtd_device_t* device);

---- program for arduino---

struct hscdtd_arduino_device_t device;

open ((struct hscdtd_device_t*) device);
...

------- end --------
In the functions themselves, the pointer to the struct would need to be casted to the platform specific definition.

void open(struct hscdtd_device_t* device)
{
   hscdtd_arduino_device_t* open_device = (struct hscdtd_arduino_device_t*)device
  open_device->a =1;
  open_device->b =2;
  open_device->c =3;
}

[bobveringa]

I am personally not a fan of core driver functionality having different behavior depending on the platform. From my experience, this is just waiting for something to go wrong. In addition, why should the hscdtd_device_t struct have any information about what fields are needed for the underlying transportation of data. It does not need to know, it does not interface with it, nor should it in my opinion.

Separating the transportation in its own struct, with separating out platform-specific information from that again, leaves the most flexibility while still keeping it relatively simple. Any shared logic is put in the transport_cookie, so there is no need to update shared logic for multiple platforms. And any special features that may be required for a given platform don't clutter up resources and logic that are shared between multiple platforms.

And while yes, the same could be achieved, by inheritance, that seems overly complicated for something that isn't. So, using both transport and platform cookies has the following advantages:

• Shared fields are only implemented once, in a clear, easy to understand method (this also helps with intelliSense)
• Platform-specific data, can only be accessed by the platform that needs it.
• Separation of concerns is also taken care, of relevant data is stored in a different struct, and it is clear to anyone using the struct, which fields are platform-specific, and which ones aren't.

Personally, I find separation of concerns is important to manage, even in small projects. Things can grow quickly, or you might want to re-use them somewhere else. This way, if you don't want to use the transportation layer, you know imminently which fields to remove.

Using the transport cookie also allows easy expendability without cluttering up the device struct. The transport cookie could be expanded with transport status like, connection open, connection closed. This could then be used to ensure that you don't try to send data when the connection is not yet open. And I feel like it would make very little sense to add this to the device struct, as it never has to deal with transportation, nor should it. The purpose of the device struct is to provide an easy-to-access container, with relevant information about the state of the chip and driver.

To be clear, if there is a clear and good argument for embedding the transportation fields into the device struct, then I will consider it. However, the argument for it would need to be pretty strong. I am also not saying that these transport and platform cookies are the way to go, but my preference is to not add platform-specific fields to the device_struct.

[tilman1]

Dear Bob,

First of all, it is your project, and if you feel that using an additional field fits your needs and design ideas best, than this is the way to go :-) And we are discussing a nuance here in my opinion.

And while yes, the same could be achieved, by inheritance, that seems overly complicated

I agree, I also feel that this adds too much complexity for the problem to be solved. I just wanted to show that other approaches also exist.

I understand that you want to strictly separate the driver from the underlying interaction with the hardware, i.e. the data transport. Only the i2c address is needed by the driver. The rest of the fields are needed by the transport layer. From that perspective, it is consequent, to introduce an additional platform specific struct.

I see the hscdtd_device_t struct more as a (platform specific) container of information needed to operate and represent the device, i.e. the sensor. And this information is different across the platforms. The arduino has only one i2c bus for instance and hence, only the i2c address is required. The rpi has 2 buses, and hence also a reference to the appropriate i2c bus is needed to accurately represent the sensor device.

I find nested structs littered across several files difficult to read and understand and hence would only employ it when it is really needed/not avoidable (and the complexity of "problem" the driver has solve, is small and hence does not justify too much structural overhead in my opinion). At the same time, I feel that the open, close ... functions with their common signatures across all platforms provide enough abstraction/separation between driver and transport layer while at the same time providing the necessary flexibility to extend it to several platforms.

That would be my reasoning -- arguably it is not very compelling and it partially represents a personal taste of mine :-)