cronologic-de / xhptdc8_babel

Wrappers, Utilities and Examples for using the xHPTDC8 with various programming languages.
Mozilla Public License 2.0
0 stars 1 forks source link

Improved User Guide Example #8

Closed sulimma closed 3 years ago

sulimma commented 3 years ago

Take the example from the User Guide and

This task is first assigned to @sulimma who will perform some refactoring and will then be assigned to @Bassem-Ramzy .

sulimma commented 3 years ago

@Bassem-Ramzy :

I refactored the example and added a few comments. I did not:

Bassem-Ramzy commented 3 years ago

Updates on function xhptdc8_init, parameter: error_message in "xHPTDC8_interface.h"

It's mentioned that:

\param **error_message is type char. The buffer for the error message has to contain at least 80 chars.

Accordingly, we need to:

  1. Validate that the returned message size doesn't exceed 80, in both driver & dummy libraries code
  2. Copy the error message (if found) to the string instead of just assigning the pointer, , in both driver & dummy libraries code
  3. Allocate the string memory in the example code
  4. Remove constant from the parameter since it will be changed if an error is encountered
    XHPTDC8_API xhptdc8_manager *xhptdc8_init(xhptdc8manager_init_parameters *params, int *error_code, const char** error_message);

    to be

    XHPTDC8_API xhptdc8_manager *xhptdc8_init(xhptdc8manager_init_parameters *params, int *error_code, char** error_message);
sulimma commented 3 years ago

I believe that this is an error in the comment. error_message is a pointer to a pointer. All possible error strings are statically assigned in the driver and the driver just overwrites the pointer with the address of the correct string. The strings are never allocated or copied.

Like this

const char * ok_msg ="OK"
const char * error_msg = "ERROR"

int driver_function(const char **msg) {
   if(error) {
     *msg = ok_msg;   //this copies the address of the string to the address given by msg. No buffer is copied.
     return XHPTDC8_OK
   } 
   *msg = error_msg; 
   return XHPTDC8_ERROR
}
Bassem-Ramzy commented 3 years ago

The comment looks consistent with the user guide:

xhptdc8_manager * xhptdc8_init(xhptdc8manager_init_parameters *params, int *error_code, char **error_message) 

Opens and initializes the xHPTDC8 board with the given index. The user must provide pointers to memory locations where the driver can store return values.

sulimma commented 3 years ago

Yes, but the return value is the address of a string, not a string. Otherwise it would be char *error_message with just a single asterics.

sulimma commented 3 years ago

I can clarify this in the user guide.

Bassem-Ramzy commented 3 years ago

In the example code:

int configure_xhptdc8(xhptdc8_manager* xhptdc8_man, int device_count) {
    xhptdc8_manager_configuration* mgr_cfg = new xhptdc8_manager_configuration;

    // configure all devices with an identical configuration
    for (int j = 0; j < device_count; j++) {
        xhptdc8_device_configuration* dconfig = new xhptdc8_device_configuration;
        xhptdc8_get_default_configuration(xhptdc8_man, dconfig);

The code logic assumes that _xhptdc8_get_defaultconfiguration() gets default configuration for a "device, _xhptdc8_deviceconfiguration", while, it gets it for a "manager, _xhptdc8_managerconfiguration" including its sub-devices configurations. Do we need in the example to have a function that gets default configuration of a device (_xhptdc8_deviceconfiguration)?

sulimma commented 3 years ago

No. That is broken. Get the default config for the manager and then loop over the array of device_cconfigurations in that structure.

Bassem-Ramzy commented 3 years ago

The function:

XHPTDC8_API int xhptdc8_count_devices(int *error_code, const char** error_message);

May be it doesn't need the hMgr as an argument in the Driver code, as it uses internal variables; but it doesn't seem consistent with other functions when being used/coded in both Dummy Library and UG Example. For consistency and systematic approach reasons, it it's recommended to take manager as a first argument

sulimma commented 3 years ago

There are a few methods that are intended to be calles before the manager object is created. For this reason they can't take the manager object as a parameter. I am currently not sure, why count_devices is on that list, but it is on purpose.

It is good that are reading the code with open eyes and detect and report such observation.

Bassem-Ramzy commented 3 years ago

Project is created, compiles, and runs properly. Might need deep testing for different business scenarios.