Keyence Driver Architecture

[Jmeyer1292]

Hello folks. I wanted to publicly float around some ideas for what the Keyence driver API should look like. I had a productive discussion with @shaun-edwards recently on this and am proposing a few options that I see and welcome other input. I'll be writing in psuedo-code and using the following helper types:

/*
  Supporting classes
*/
class ByteBuffer; // some storage class for IO
class Connection; // some sort of generic IO handle

Here's my initial idea, inspired by work I've done in the past with MavLink (https://github.com/mavlink/mavlink):

/*
  Sample 1: C-esque API
*/
struct ChangeProgramRequest; // message fields for request
struct ChangeProgramResponse; // and response

void encodeChangeProgramRequest(const ChangeProgramRequest&, ByteBuffer&);

void decodeChangeProgramResponse(const ByteBuffer&, ChangeProgramResponse&);
// maybe
ChangeProgramResponse changeProgram(const Connection& s, const ChangeProgramRequest&);

Every command would exist in its own file and besides some common helper functions, every one stands on its own. This design breaks up the encoding and decoding of messages so they can be handled separately (if desired) or the last function could be used to a synchronous send/receive.

A usage case would like the following:

ChangeProgramRequest req;
req.program_number = 1;
// or it could be encoded, decoded by the programmer however he sees fit
ChangeProgramResponse res = changeProgram(keyence, req);

@shaun-edwards suggested a more object oriented approach, which I interpreted as the following (but please critique):

/*
  Sample 2: Messages with interfaces
*/
class KeyenceMessage {
public:
  virtual void encodeRequest(ByteBuffer&);
  virtual void decodeResponse(const ByteBuffer&);
};

class ChangeProgram : public KeyenceMessage {
public:
  virtual void encodeRequest(ByteBuffer&) override;
  virtual void decodeResponse(const ByteBuffer&) override;

  struct ChangeProgramRequest; // or equivalent fields
  struct ChangeProgramResponse;
};

class KeyenceConnection {
public:
  void sendRecieve(KeyenceMessage& msg) {
    // Send request
    ByteBuffer b;
    msg.encodeRequest(b);
    socketSend(b);
    // Recieve response
    ByteBuffer r = getResponse();
    msg.decodeResponse(r);
  }
};

This methodology uses inheritance to enforce the interface. And leads to different usage:

KeyenceConnection keyence; // some networking info here obviously
ChangeProgram msg (1);
keyence.sendRecieve(msg);

I think that this API is arguably more natural but it would require changes to the connection class to handle messages in a different way and it's generally a bit more complex.

Last but not least:

/*
  Sample 3: All one object
*/
class Keyence {
public:
  void changeProgram(uint8_t program_number); // would call sample 1 or 2 under; throw exception if failure
};

Since we're dealing with a single communication protocol here and if we're aiming for usability over flexibility, then why not go all the way and hide the IO completely (we would still use one of the other APIs and we could expose them if a user wanted to get low level):

Keyence keyence; // network info in constructor
keyence.changeProgram(1); // throws exeception on failure to change program

I envision exceptions being used in all of these methods but we could return error codes if we want more portability or cooperation of unit testing tools.

I would also envision writing a ROS node that wraps the above library and exposes services/topics analogous to those supported by the API.

@shaun-edwards @gavanderhoorn @jrgnicho Please look.

[gavanderhoorn]

For an internal (de)serialisation hierarchy of C++ classes I think I like option 2 best (seems a bit like simple_message works?).

Option 3 hides too much: it should be obvious that there is network business going on: this would be a developer interface and encapsulation of that part of the drivers behaviour behind a facade like that doesn't feel right. I may want to interact directly with the device / connection, and option 3 would require me to first implement an additional method and / or add an accessor for the data I want which seems to complicate things unnecessarily.

In the past, @shaun-edwards and I discussed structuring the driver internally to use the -- already documented for us -- Keyence API. I think that makes sense and I think it would be nice to do that. It is C only though, but we could of course just add it as a sort of shim (extern C and all that). Alternatively we could keep the entire thing in C.

In all cases I suggest we follow the ros-agnostic-library-then-wrap-in-a-driver-node approach. That will probably require us to re-implement some things that we could otherwise borrow from ROS, but it should be worth it.

[Jmeyer1292]

My only immediate reservation with the current Keyence API is that it seems to follow the singleton pattern.

You call LONG LJV7IF_Initialize(void) then LONG LJV7IF_EthernetOpen(LONG IDeviceId, LJV7IF_ETHERNET_CONFIG* pEthernetConfig);, etc...

We could accept that and allow one connection per process. We could augment the functions to take a pointer to some connection object we invent as a first argument. Thus we make minimal changes but retain flexibility. We could park the functionality in an object, but that's pretty damn close to option 3 from my original post.

[shaun-edwards]

I like keeping the same API. I always figured we would keep it object oriented, but use the same method names. Having exactly the same API would make our code a drop in replacement, although I don't have high hopes that this would ever happen. My main motivation for keeping the same API was avoiding documentation :)

Of course, I prefer the simple_message approach...I guess that goes without saying. I also believe in creating a library, and then wrapping it in a lightweight ROS node. Creating a ROS agnostic library is less of a requirement and can be difficult. For example, ROS logging is very convenient. We should endeavor to use common/portable libraries instead of ROS though. For me, being ROS independent means not using the messaging capability.

[gavanderhoorn]

@Jmeyer1292 wrote:

We could accept that and allow one connection per process. [..]

The keyence lib actually supports multiple connections per program. The LONG you get back (IDeviceId) is simply an identifier that is used to indicate to the API functions which connection you want to address. It indexes into an internal table.

I'm not saying that is necessarily the way to go though.

We could augment the functions to take a pointer to some connection object we invent as a first argument [..]

We could replace the IDeviceId parameter with a per-connection struct, similar to how the Linux kernel works (device data). That would remove the need for dll-global shared data and support an unlimited nr of connections.

[Jmeyer1292]

Where do you get IDeviceId from?

Do you choose it when establishing comms? Those LONG returns are just error codes. Every single function returns LONG.

[gavanderhoorn]

@shaun-edwards wrote:

I always figured we would keep it object oriented, but use the same method names. Having exactly the same API would make our code a drop in replacement, although I don't have high hopes that this would ever happen. My main motivation for keeping the same API was avoiding documentation :)

I don't even really think OO is needed. It is convenient, but for what it offers here I think we should be able to live without it. If I understand correctly, the inheritance tree would only be two layers deep: base message class and then all specialisations for each message. The protocol isn't that complex and I don't think it requires more than that.

The 'union approach' would also be an option: it is actually used by the motoman_driver on the MotoPlus side to implement the simple_message (de)serialiser (see MotoPlus/SimpleMessage.h).

re: documentation: exactly. Drop-in replacement: not so much.

[gavanderhoorn]

@Jmeyer1292:

9.2.9.1 Communication devices The controller that will communication with the PC is specified as a communication device. The maximum number of controllers that can be communicated with simultaneously is defined by LJV7IF_DEVICE_COUNT (8.1 Constant definitions). In interfaces that involve communication, you can specify the controller to target for communication with IDeviceID. IDeviceID can be specified as 0 to (LJV7IF_DEVICE_COUNT-1).

From the Keyence - LJ-V7000 Series Communication Library Reference Manual.

essentially you just make it up.

[gavanderhoorn]

Hm, re-reading @Jmeyer1292's first post it would seem Option 1 makes most sense for a keyence-like api. But again, the C-api could be added as a shim.

[Jmeyer1292]

We could use something akin to option 1 under the hood and then wrap it with an interface that copies the existing Keyence one. I think we could go for drop in compatibility.

How would we handle comms? Do we want to write a small wrapper around the linux socket libraries?

[shaun-edwards]

@Jmeyer1292 wrote:

How would we handle comms? Do we want to write a small wrapper around the linux socket libraries?

That's essentially what the socket library in ROS-I does. Linux sockets are way too complicated for any mere mortal to use (although we know the @Jmeyer1292 is no mortal). Seriously, linux sockets are pain to use and result in error prone code. We might consider a more full featured library that reduces socked complexity...see here.

[gavanderhoorn]

@Jmeyer1292: there are many socket libs in existence. I'm ok with using one, as long as it doesn't introduce too many additional dependencies.

@shaun-edwards: linux sockets are ok, don't exaggerate :). It's not too hard to setup a simply socket connection, and the keyence device doesn't need too much.

[Jmeyer1292]

We can hash out exactly what we want our communication layer to look like... I think something simple that just supports synchronous read/write is all we need.

My last question for the moment is one you already touched on @gavanderhoorn: Do we make this a C++ library with a C interface or do we go for C throughout?