0

Recently I had my first experience writing a wrapper around FTD2XX driver for a desktop tool that communicates with an embedded device. The library provided some common device specific functionality and encapsulated low-level MPSEE and board specific configurations from the user.

While writing this library I found myself choosing between two types of APIs that the library provides

  1. Function X takes in specific arguments that it needs and nothing more like the following

    eSTATUS spi_init(FT_HANDLE ftdi_handle, eFREQUENCY clock_freq)

  2. Function X takes in a control block structure that encapsulates some specific state of the device (previously configured or otherwise)

    eSTATUS spi_init(sDEVICE_CBLK cblk, eFREQUENCY clock_freq) // where sDEVICE_CBLK is a struct containing a handle as well as other device-related fields

The function does the same thing in both cases, but API looks fairly different.

Looking around at various examples I see both approaches as well as mixed ones. My thinking is that approach 1 is more self documenting and "lightweight", in that you only supply what is needed, but approach 2 is a lot easier to refactor in the future (in case of adding a field to a struct) but then it somehow seems less "aesthetically pleasing" to me, in that it's nowhere near as self documenting

What I found is that some functions may get away with approach 1, while others require device state to operate correctly (such as GPIO directions), but then I end up with heterogeneous API which I would like to avoid if possible.

What I strive for is an API that is intuitive and easy to use and easy to refactor if needed. ABI backwards compatibility isn't a requirement in my case

It seems to me that approach #2 is far more practical and clean - nearly every function will have the same API, refactoring becomes easier and it makes sense that such library provides a control block structure for user to initialize and manage through provided API.

Is there a preferred approach in such situations? I understand this is very open-ended so I would appreciate any thoughts on the matter

3

Approach 1 is simpler. Approach 2 adds complexity for the sole purpose of, maybe, making it easier to change something in the future. This is questionable because:

  1. Most software gets written and then never gets significantly updated. Its a waste of time to add extra code and complexity that may never be used.

  2. Even if the software will gets revised in the future, it could be in an unanticipated direction that cannot use the current approach 2. Maybe multiple FTD2XX chips will be added. Perhaps a system power management function is added which then requires an extra system power struct parameter.

  3. New revisions of a product often use new chips, sometimes from different vendors. New chips will likely have a different Api. Approach 2 will also fail here.

Keep it simple is a good guideline.

4
  • 1
    eSTATUS spi_init(open_firmware_hell_t *device_tree_handle); Mouhahaha!
    – Kaz
    Jun 22 '21 at 0:36
  • While I agree in principle to keep thing simple, it seems that approach #2 is indeed simple in many respects. It is often the case that a specific driver library exposes some sort of a control block structure anyway, this is because it is often the case that a driver encapsulating device operation is stateful, so you need to save state of the device somehow, and such control block structs provide an organized way of storing it. Since its already there, why not re-use it as a parameter?
    – keyermoond
    Jun 22 '21 at 19:07
  • I am not sure what you mean by your second point. These kinds of structs usually contain stateful information only, for example - state of your GPIO bank direction configuration 8 bits indicating what's input/output. If there is a parameter that enables/disables power, it wouldn't be part of such a struct. If new chips are added that have different API then they require their own drivers? Why would it be reasonable to have one driver be able to service both types of chips? Approach #1 may fail here as well ie SPI_Init now also requires slave polarity or something
    – keyermoond
    Jun 22 '21 at 19:17
  • @Kaz this is precisely what I would like to avoid, I've seen several examples where there is a struct with 20 fields being passed around all functions in the driver. But on the other hand, keeping only stateful info in such a struct and keeping everything else as a seperate parameter seems reasonable to me. In the example you give the device handle may go into a struct, but say slave polarity or clock phase or w/e will have their own parameter fields I had difficulties trying to find any discussions on this specific topic, hard to formulate a good query that would result in anything useful
    – keyermoond
    Jun 22 '21 at 19:23
2

If the driver functions need the instance structure DEVICE_CBLK, but it is not specified in spi_init as an argument, or as a return value, that means it is a global variable somewhere inside the driver. This means that the driver can only support a single instance of the device. It might be taking multiple FT_HANDLE arguments, but that may not be usable for talking to multiple devices, if they need separate state information in the DEVICE_CBLK.

Either of these allows the maximum flexibility in multiple instantiation:

// allocate and return a new DEVICE_CBLK, used for all other calls
DEVICE_CBLK *spi_new(FT_HANDLE ftdi_handle, FREQUENCY clock_freq,
                     STATUS *pStatus);

// alternative: initialize client-allocated control block
STATUS spi_init(DEVICE_CBLK *cblk, FREQUENCY clock_freq);

spi_new pairs with spi_destroy; spi_init pairs with spi_cleanup.

Supporting multiple instantiation via an instance structure that is passed around is very easy to achieve, with only a small amount of start-up effort which starts paying for itself almost immediately.

Suppose you are sure that you don't need it though. You can still do this:

// in header file typedef struct DEVICE_CBLK DEVICE_CBLK;

In the implementation:

DEVICE_CBLK *spi_new(FT_HANDLE ftdi_handle, FREQUENCY clock_freq,
                    STATUS *pStatus)
{
  spi_clock_freq = clock_freq;
  if (spi_open) {
    *pStatus = ERROR_DEVICE_BUSY;
    return NULL;
  }
  spi_open = 1;
  return (DEVICE_CBLK *) ftdi_handle;
}

void spi_destroy(DEVICE_CBLK *cblk)
{
  // nothing to free here; FT_HANDLE is not our responsibility
  if (spi_open) {
    // cleanup
    spi_open = 0;
  }
}

In other words, we lied; there is no device block. We didn't allocate anything in spi_new; we just converted the FT_HANDLE pointer to DEVICE_CBLK *. All the other constructor parameters went into global variables.

Our functions still take a DEVICE_CBLK *, but convert it to a FT_HANDLE for calling the lower level SPI routines that need a handle.

But we have enough of an abstraction to take place to go to a real DEVICE_CBLK in the future without changing the client code, or even recompiling it.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.