REDLINK SDK v6.31.6 Released

[Mikael Lubtchansky]

REDLINK SDK - continued.

rcp_api_hw_cap.c

+) _rcp_hw_cap_handle_older_firmware()

Currently:

/* If we have multi monitor feed 1 and 3D LUT support set
* the meta capability HW_CAP_MULTI_MONITOR_FEED_1_AND_3D_LUT */

if (_rcp_get_hw_cap(con, HW_CAP_MULTI_MONITOR_FEED_1) &&
_rcp_get_hw_cap(con, HW_CAP_MULTI_MONITOR_FEED_1))

Comment: I believe there is code error here - suggest replacing this with:

/* If we have multi monitor feed 1 and 3D LUT support set
* the meta capability HW_CAP_MULTI_MONITOR_FEED_1_AND_3D_LUT */

if (_rcp_get_hw_cap(con, HW_CAP_MULTI_MONITOR_FEED_0) &&
_rcp_get_hw_cap(con, HW_CAP_MULTI_MONITOR_FEED_1))

AJ

It does look like there is a code error in this 'older hardware' support routine... as it is indeed checking twice for same capacity,
but to make sense, the correct code should check for HW_CAP_3D_LUT and not HW_CAP_MULTI_MONITOR_FEED_0 :

if (_rcp_get_hw_cap(con, HW_CAP_MULTI_MONITOR_FEED_1) && _rcp_get_hw_cap(con, [B][COLOR="#FF0000"]HW_CAP_3D_LUT[/COLOR][/B]))
   {
     _rcp_set_hw_cap(con, HW_CAP_MULTI_MONITOR_FEED_1_AND_3D_LUT, 1);
    }
 else
   {
     _rcp_set_hw_cap(con, HW_CAP_MULTI_MONITOR_FEED_1_AND_3D_LUT, 0);
    }

Mikael

 

[Antony Newman]

Thanks Mikael,



AJ

 

[Trent Lillehaugen]

Thank you for pointing out the bug in rcp_api_hw_cap.c, it will be addressed in a future release.

Antony, I will come back and comment on your other observations when time permits.

 

[Antony Newman]

Thanks Trent,

AJ

 

[Antony Newman]

REDLINK SDK - continued.

rcp_api_camera_connection.c

+) The codebase uses of '-1' where '_state' variables are not_yet_set.
These '_state' variables are then compared with enums to verify currents.
And '-1' is not a listed in the enum definition.

Eg:
con->last_record_state = -1;
con->last_magnify_state = -1;
con->last_playback_state = -1;
...
if ( ((con->last_record_state == RECORD_STATE_NOT_RECORDING) && (state->val.int_val != RECORD_STATE_NOT_RECORDING)) ||
((con->last_record_state != RECORD_STATE_NOT_RECORDING) && (state->val.int_val == RECORD_STATE_NOT_RECORDING) )
...
typedef enum <from rcp_types_public.h>
{
RECORD_STATE_NOT_RECORDING,
RECORD_STATE_RECORDING,
RECORD_STATE_FINALIZING,
RECORD_STATE_PRE_RECORDING,
RECORD_STATE_ENCODING
} record_state_t;

Comment_1 : Suggest that an additional enum state in introduced into each respective '_state' enum to represent 'not yet set'

Eg :
typedef enum
{
RECORD_STATE_NONE,
RECORD_STATE_NOT_RECORDING,
RECORD_STATE_RECORDING,
RECORD_STATE_FINALIZING,
RECORD_STATE_PRE_RECORDING,
RECORD_STATE_ENCODING
} record_state_t;

Comment_2 : This would then permit the definitions of the various state variables in 'rcp_camera_connection_t' to be enums.

Currently : int32_t last_record_state;
Suggested : record_state_t last_record_state;


AJ

 

[Antony Newman]

REDLINK SDK - continued.

rcp_api_parameters.c

+) _rcp_param_def[] :
Currently :
{STR(RCP_PARAM_AUDIO_CH34_STATE), MIN_VER(5, 0), LABEL("CH32 State"),
Comment : I guess this is supposed to be:
{STR(RCP_PARAM_AUDIO_CH34_STATE), MIN_VER(5, 0), LABEL("CH34 State"),

+) _rcp_param_def[] :
Currently :
{STR(RCP_PARAM_AUDIO_CH12_LINK_VOL), MIN_VER(6, 0), LABEL("Audio: CH2/2 Link Volume"),
Comment : I guess this is supposed to be:
{STR(RCP_PARAM_AUDIO_CH12_LINK_VOL), MIN_VER(6, 0), LABEL("Audio: CH1/2 Link Volume"),


AJ

 

[Antony Newman]

REDLINK SDK - continued.

<RESOVLED>

rcp_app_menu.c

+) _rcp_menu_send_children() :

USE CASE :

APP calls rcp_menu_send_children() with a MENU_NODE_ID that is valid (ie Parent Menu already stored)

rcp_menu_send_children() : parent = _rcp_menu_find_parent(con, id)
(so at this point 'parent' will point to a valid 'rcp_menu_parent_node_list_t' struct)

_rcp_menu_send_children() : if (con && parent && con->cur_menu_cb) {
(this condition is satisfied).


Comment : At this point I would have thought that all the information
to create 'rcp_cur_menu_cb_data_t data;' was available - and that:
i) 'data' would be populated, and that
ii) the APP callback con->cur_menu_cb could be called.

However: There is conditional step in the code in _rcp_menu_send_children() :
if (!playback_state->val_valid)
{
(void) _rcp_get(con, RCP_PARAM_PLAYBACK_STATE);
return;
}

Thoughts : I am uncertain why - if the APP has requested the menu structure, and this is available,
why the PLAYBACK_STATE is being requested from the camera before giving the the APP the menu structure it requested.


AJ

EDIT : Just realised that Changing PLAYBACK state changes the way the Menu's are 'filtered' before a selection is presented to the user.

<RESOVLED>

 

[Trent Lillehaugen]

Antony,


Again, I will forward the documentation comments on. I have addressed some of the more technical issues below. Most of the issues you have found will be incorporated in a future release of the API. Thank you for pointing out any issues.


I would like to stress that the API should be used as a black box. That is, you should use the API as if you only had access to "rcp_api.h". That doesn't mean you shouldn't look at the source to gain understanding or to find bugs for us , but you shouldn't rely on the current implementation staying as it is. You should only include "rcp_api.h" in your code (that is, don't include our 'private' headers), and only access members of types we have declared there. This will help ensure compatibility with your application and future releases of the API.


We provide the source so that the API can be ported to different platforms without the need for us to explicitly support it and generate the required binaries.


That being said, you are free to inspect the code. And please continue to report any issues you may find.

+) Page 30 : 'RCP_API_ENABLE_CACHEING '
(very minor) Typo : Other places in the documentation - Caching (correct spelling) is used. I note this might be tricky to fix

Apparently I can't spell 'caching'. This will be corrected in the code in a future release.

REDLINK SDK - Started reading
General comments : - and apologies if I mess up ... I'm a complete newbie to Objective C!

The REDLINK SDK is written in C (with a few helper classes written in C++). None of it is written in Objective C (except for our sample iOS code - which is not part of the SDK proper).

clist.h
Comment : The inclusion of <stdlib.h> and <stdint.h> are redundant (as c_list.h already includes them)

Yes, they are redundant, but that is not a problem. I try not to rely on included header files to include additional headers that are required for any given module. I would rather explicitly include them (even if it is redundant).

c_list.h
c_list_import_from_string() "This function will fill the import a string created by @ref c_list_export_to_string."
Comment : Suggest rewording this to "This function will populate a list with concatenated strings previously created by @ref c_list_export_to_string."

The existing comment isn't a coherent sentence. It will be fixed in a future release.

Introductory Banner: "list.cpp"
(very minor) Comment : (caveat I've never seen C++ before) Guess this should be "clist.cpp"

This has nothing to do with C++, it is simply a typo. It will be fixed in a future release.

File Compression :


The REDLINK API development guide mentions that Zlib is used in camera to compress/decompress files.


Apple 2015 WWDC : Session 102 (Platforms State of the Union) discussed Zlib, and Apples wish to create a:
+) Faster Compress / Decompress algorithm
+) With higher compression ratio than Zlib
+) That was optimised for modern computer architectures (by which I am guessing they mean ARM v8 ISA)


They utilised Finite State Entropy (FSE) to create a proprietary Apple library (OSX and iOS) called LZFSE with:
+) 3 x speed increase when encoding
+) 60 % lower energy used.


This lead me to see if I could find an open source version of FSE that RED may wish to review - as a reduction in camera energy footprint / faster encode - may also be of interest.


The best summary I found was here:
https://lyncd.com/2015/09/lossless-compression-innovation/


ZStandard : Is Yann Collet's open source of an FSE that is 2 x faster than Zlib (ie less CPU), with similar compression rations.


Broti : Is Googles open source of an FSE implementation that is aims at better compression (but with more CPU)


.. and so in summary, if RED are looking for a successor to Zlib for the SDK, I would recommend reviewing ZStandard.


AJ

We have no intentions on changing the compression algorithm at this time.

rcp_type_public.h


+) /* RCP: CH12SRC, CH43SRC */
(minor) Comment : Amend to /* RCP: CH12SRC, CH34SRC */ to match page 45 of the RCP PDF.


+) /* RCP: SHMODE */ typedef enum { //selected shutter sync mode ... } shutter_mode_t;
... /* RCP: SHMODE */ typedefenum { SHUTTER_MODE_ABSOLUTE, ... } sync_mode_t;
Comment : Page 19 of RCP PDF advises that SHMODE should relate to 'shutter_mode_t'.
Suggestion : Amend SHMODE to SHSYNC mentioned on page 29 of the RCP PDF.

These will be fixed in a future release.

+) 'T' /* Set List command */
'F' /* Fail */
Comment : Suggest that REDLINK Command Protocol Reference Guide.pdf page 10 is updated to include one or both of these commands
(not sure if 'F' is useful for at this point).

Yes, 'T' should be documented. 'F' is not used as part of the public RCP protocol. We should perhaps document that it is not used.

+) typedef enum{ ... } tRCPState;
Comment : This is the first occurrence of an enum where the the name does not end in a '_t'.
Comment 2 : At this point I am uncertain what the significance of the '_t' is for all the other enums.
(this may well have something to with me being new to C++).

This is simply a naming convention. In general we will end all typedef'd types (enums, structs, etc) with "_t". tRCPState, for whatever reason, didn't follow this naming convention. We haven't changed it as third party code could be using it and we we'd rather keep backwards compatibility.

+) "Set initial values for Parsing (We need to initialize the parser so we can runThe checksum Validation"
(Very minor) Comment : Suggest updating to:
"Set initial values for Parsing (We need to initialize the parser so we can run
the checksum Validation)"


+) int RCP_validateChecksum(const tRCP * pRcpData);/* Validate checksum , calculates the binary checksum
(minor) Comment : Suggest updating to * Validate checksum binary, calculates the binary checksum
Reason : This C comment relates to the type definition below - but happens the same words of the procedure name above it.

Comments will be cleaned up in a future release.

+) RCP_get_and_parse_packet2(): printf("Testing RCP_get_and_parse_packet:\n");
(minor) Comment : Amend to printf("Testing RCP_get_and_parse_packet2:\n");
Reason : The routine RCP_get_and_parse_packet() is located below this one, and both *packet() and *packet2() are called by the test code.

There is no RCP_get_and_parse_packet2. test_RCP_get_and_parse_packet and test_RCP_get_and_parse_packet2 are both testing RCP_get_and_parse_packet.

rcp_api_state.c


+) _rcp_set_state()


Comment : The code clears cached values for most values in the rcp_param_state_t struct.


The following to variables are not included:
unsigned int is_hw_supported : 1;
unsigned int is_hw_supported_valid : 1;


Noted also that : Union variables are (also) not zeroed.


Suggestion : amend _rcp_set_state() to include is_hw_supported & is_hw_supported_valid


Suggestion 2 : If these specific variables are not cached then explicitly note this in the code.

Thanks, is_hw_supported and is_hw_supported_valid should be cleared here. Although, they are correctly initialized in rcp_create_camera_connection.


I'm not sure what "union variables" you are referring to. Are you talking about the val and target_val values? Those don't need to be cleared as the val_valid and target_val_valid flags are cleared.

Continued...

 

[Trent Lillehaugen]

...Continued: apparently REDUSER has a 10000 character limit per post

rcp_api.h


Just noting my thoughts here - as there is probably something I am missing :


typedef struct
{ ...
rcp_camera_connection_t * con;
const rcp_cam_info_t * cam_info;
} rcp_state_data_t;


struct rcp_camera_connection_t (defined in rcp_api_camera_connection.h)
{ ...


rcp_cam_info_t cam_info;
}


Comment : 'rcp_camera_connect_t' has the 'rcp_cam_info_t' structure.
'rcp_state_date_t' has a pointer ('con') to both this structure,
and maintains a pointer to the 'rcp_cam_info_t' (cam_info).


Suggestion : Consider removing "const rcp_cam_info_t * cam_info;" from rcp_state_data_t and update code to get this directly from rcp_camera_connection_t


Caveat : There maybe something I am missing as to why it was done this way...

From user code (your code), you cannot directly see the members of rcp_camera_connection_t as it is an opaque type. That is, rcp_api.h gives this declaration:
typedef struct rcp_camera_connection_t rcp_camera_connection_t;


This is similar to something like FILE in stdio.h.


However, since we provide the source code you can see the details under the hood.

rcp_api_camera_connection.c


+) rcp_create_camera_connection() : con->state = (rcp_connection_state_t) -1;


Comment : rcp_connection_state_t is a typedef enum.
Suggestion : Replace (rcp_connection_state_t) -1 with a valid RCP_CONNECTION_STATE_...

You are correct, this is an abuse of the enum and should be corrected.

rcp_api_get.c


+) _rcp_get() : first occurrence of if (!has_get)
Currently :
if (def->create_int_fp)
{
...
}


if (def->create_string_fp)
{
...
}


Comment : I had a doubt here. If the rcp_parm_def_t[] structure in rcp_api_parameters.c happens to have functional pointers for both
'create_int' and 'create_str' this would cause the calling of create_int_fp() and _rcp_send_current_composite_string() to both be called.


Comment 2 : As _rcp_param_def[] is defined using macros represent one or several parameters, making somewhat difficult to determine
which parameters in _rcp_param_def[] this might apply to for static analysis purposes.


Suggestion : If only one clause is expected to run (and in the avoidance of doubt) the following could be considered:
if (def->create_int_fp)
{
...
}
else if (def->create_string_fp)
{
...
}

There is no reson a parameter couldn't have both set, and there is no reason both shouldn't be called if they were.

rcp_api_hw_cap.c


+) _rcp_hw_cap_handle_older_firmware()


Currently:


/* If we have multi monitor feed 1 and 3D LUT support set
* the meta capability HW_CAP_MULTI_MONITOR_FEED_1_AND_3D_LUT */


if (_rcp_get_hw_cap(con, HW_CAP_MULTI_MONITOR_FEED_1) &&
_rcp_get_hw_cap(con, HW_CAP_MULTI_MONITOR_FEED_1))


Comment: I believe there is code error here - suggest replacing this with:


/* If we have multi monitor feed 1 and 3D LUT support set
* the meta capability HW_CAP_MULTI_MONITOR_FEED_1_AND_3D_LUT */


if (_rcp_get_hw_cap(con, HW_CAP_MULTI_MONITOR_FEED_0) &&
_rcp_get_hw_cap(con, HW_CAP_MULTI_MONITOR_FEED_1))

As discussed earlier, we have fixed this and it will be available in a future release. Note, this would only affect Weapon cameras running 6.1.x.

rcp_api_camera_connection.c


+) The codebase uses of '-1' where '_state' variables are not_yet_set.
These '_state' variables are then compared with enums to verify currents.
And '-1' is not a listed in the enum definition.


Eg:
con->last_record_state = -1;
con->last_magnify_state = -1;
con->last_playback_state = -1;
...
if ( ((con->last_record_state == RECORD_STATE_NOT_RECORDING) && (state->val.int_val != RECORD_STATE_NOT_RECORDING)) ||
((con->last_record_state != RECORD_STATE_NOT_RECORDING) && (state->val.int_val == RECORD_STATE_NOT_RECORDING) )
...
typedef enum <from rcp_types_public.h>
{
RECORD_STATE_NOT_RECORDING,
RECORD_STATE_RECORDING,
RECORD_STATE_FINALIZING,
RECORD_STATE_PRE_RECORDING,
RECORD_STATE_ENCODING
} record_state_t;


Comment_1 : Suggest that an additional enum state in introduced into each respective '_state' enum to represent 'not yet set'


Eg :
typedef enum
{
RECORD_STATE_NONE,
RECORD_STATE_NOT_RECORDING,
RECORD_STATE_RECORDING,
RECORD_STATE_FINALIZING,
RECORD_STATE_PRE_RECORDING,
RECORD_STATE_ENCODING
} record_state_t;


Comment_2 : This would then permit the definitions of the various state variables in 'rcp_camera_connection_t' to be enums.


Currently : int32_t last_record_state;
Suggested : record_state_t last_record_state;

Again, this is an abuse of the enum system and should be cleaned up. However, the code is functionally correct.

rcp_api_parameters.c


+) _rcp_param_def[] :
Currently :
{STR(RCP_PARAM_AUDIO_CH34_STATE), MIN_VER(5, 0), LABEL("CH32 State"),
Comment : I guess this is supposed to be:
{STR(RCP_PARAM_AUDIO_CH34_STATE), MIN_VER(5, 0), LABEL("CH34 State"),


+) _rcp_param_def[] :
Currently :
{STR(RCP_PARAM_AUDIO_CH12_LINK_VOL), MIN_VER(6, 0), LABEL("Audio: CH2/2 Link Volume"),
Comment : I guess this is supposed to be:
{STR(RCP_PARAM_AUDIO_CH12_LINK_VOL), MIN_VER(6, 0), LABEL("Audio: CH1/2 Link Volume"),

Thank you, these typos will be corrected in a future release.

 

[Wil Wong]

Gee, you guys talk funny in this thread. As long as the steak taste good, keep talking your MATRIX language and I'll keep taking the "RED" pill... bahaha

 

[Antony Newman]

Haha .... if you think this looks like the Matrix ... you should see my handwriting! ;-)

When get around to asking 'what would you like to see' - I promise to use language a bit more 'down to earth.

AJ

Gee, you guys talk funny in this thread. As long as the steak taste good, keep talking your MATRIX language and I'll keep taking the "RED" pill... bahaha

 

[Antony Newman]

Thanks for the feedback Trent.

I thought I would take a peek under the hood before I get behind the steering wheel and learn to drive.

AJ

 

[Antony Newman]

Reference : For any other developers whom (also!) have never come across Objective C before (ie the language used in the SDK iPhone Example program is written):

Here is an free series of iTunesU lectures from a Stanford Uni course: on iPad and iPhone Application Development

https://itunes.apple.com/itunes-u/ipad-iphone-application-development/id473757255?mt=10

AJ