REDLINK SDK v6.31.6 Released

[Trent Lillehaugen]

REDLINK SDK v6.31.x is a minor update to REDLINK SDK v6.30.x, adding support for the new RAW Zebras feature.

REDLINK SDK V6.31.1 CHANGES

- Bug Fixes
- Added parameters:
RCP_PARAM_ZEBRA_1_MODE
RCP_PARAM_ZEBRA_1_STOPS_LOW
RCP_PARAM_ZEBRA_1_STOPS_HIGH
RCP_PARAM_ZEBRA_2_MODE
RCP_PARAM_ZEBRA_2_STOPS_LOW
RCP_PARAM_ZEBRA_2_STOPS_HIGH

Always check https://www.red.com/developers for the most up to date version of the SDK.

 

[Antony Newman]

Hi Trent (and gang).

Just started reading the Redlink Command Protocol Red Guide (910-0046 Rev P) - and will list my observations here.

+) Page 8 : "Select the Enable WiFi check box. " : I don't have a camera in front of me - just looking at the screenshot. I note that there is no checkbox (It may well be that the screenshot only appears after a checkbox was clicked in)

+) Page 9 : "32 bit value with the major version in the high word and minor in the low" : I will assume that 'word' means 16bits for the RED SDK.

+) Page 9 : <Graphic structure> My PDF read shows the last character before the 'Checksum' bubble as a Single Dot (at the height of an apostrophe ).
However the Comment box for ': Separator. The separator separates all elements of the message betweenthe source ID and the checksum. The last character of the message beforethe checksum shall be a separator '

[Question] Is the 'single dot graphic' is incorrect - was this supposed to be a ':' separator?
[Question] If this was supposed to be separator - then - as the character before this is Also a separator - RCP messages would end with either
: : Checksum <LF> or
: <LF> (in the case where there is no Checksum).

EDIT : SOLVED : SEE Comment for page 10

+) Page 9 : ‘$’ followed by Source ID name. This identifies the sender of the message. The source ID can be up to eight (8) alphanumeric characters long. Use‘EXT’ for creating messages to the camera.'
Comment : I found this a somewhat confusing way of presenting information.
What about. "The SourceID is a 0-8 character alphanumeric string representing the device transmitting the RCP message. When this is a camera, the string defined in Menu >Settings > Setup > Communications menu will be used; When an external device, use 'EXT'."

+) Page 10 : The information about the '.' appears on page 10.
Suggestion : In the flow diagram change -> . -> Checksum -> <LF> change this to imply that the '.' part of the checksum.
Suggestion : The small font size of this character in the Graphical flow made it look more like a ' on my screen - suggest that this * is made a little bigger?

+) Page 10 : In table - where single characters are listed to the right of 'Command' : Suggest using 'G' rather than G as this convention is used thus far in this document.

+) Page 10 : Suggestion : In table change "GET, SET, SET_RELATIVE, SET_LIST_RELATIVE,CURRENT, GET_LIST, and CURRENT_LIST " to " GET, GET_LIST,SET, SET_RELATIVE, SET_LIST_RELATIVE,CURRENT, and CURRENT_LIST ' to retrospectively follow the order of the single character information.

+) Page 12 : "POSTFIX: a decorated string to be shown behind the value such as a small ‘k’ for color temperature."
Suggestion : Replace 'behind' with 'after'

+) Page 12 : "An example using the FLUT parameter is below. Its natural range is -8.0 to +8.0, the selected index is the160th being the last or 8.0 and can be specified in steps of 0.001. There are no prefix or postfix strings. #$SCARLET:FLUT:1|160|-8000|8000|100|1000|1|||: "
Comment : Reading this .. my first thought is that misread the definition of PRECISION / DIVIDER and whether what would be displayed to the user would be -8.0 or -8.000.
Perhaps an example of #$SCARLET:FLUT:1|159|-8000|8000|100|1000|1|||: would make this more clear? With a value of +7.999

The ambiguity comes from the definition of 'PRECISION' in the sentence 'PRECISION: the smallest natural value increment. With a DIVIDER of 1000 and PRECISION of 1, the natural value is represented to 0.001. ". I was not sure if this sentence was telling me Log(Divider)/Log(10) = 3 .. and there the value presented to the user should have 3 significant figures ... OR that you 3 decimal places were desired. +8.000 would have been what I was expecting .. but +8.0 is what was listed in the example.

AJ

 

[Antony Newman]

... continued.

Redlink Command Protocol Red Guide (910-0046 Rev P)

+) Page 12 : "the decorated display string for 1/48 of a second would be “&red1over;48&redsec;”
Suggestion : Replace with "the decorated display string for "1/48sec" would be “&red1over;48&redsec;”

+) Page 13 : (Special Aggregate Type) "This applies to the value types MSG_AUDIOVU_DATA"
Guess : This used to be a value type - but was depreciated. I could not find further references of this in this document or the SDK files.

+) Page 13 : (Special Aggregate Type) "These are also provided in the C header file that RED supplies."
Comment : My first reaction to this was to search for 'MSG_CLIPINFO_PTR' in the SDK files. Then each of the other 4 value types in the C files.
The only references of the these types (that I found) were actually on pages 14 & 33 of this PDF.
The Table on that page then has a reference to a 'SYSTEM GROUP PARAMETER' - And it is this that is located in the C files.

+) Page 14 : "The S, G, and C columns indicate whichcommands are supported for a given parameter. "
But on page 10 : "This document will use GET, SET, SET_RELATIVE, SET_LIST_RELATIVE,CURRENT, GET_LIST, and CURRENT_LIST to represent the command intext, "

Comment : At this stage in reading the document it had to scratch my head for a few seconds wondering what 'S','G' and 'C' stood for.
Once you know ... you know ... but you might what to consider replacing this with "The 'S' (SET) , 'G' (GET), and 'C' (CURRENT) columns indicate which commands are supported for a given parameter. "


AJ

 

[Antony Newman]

... continued.

Redlink Command Protocol Red Guide (910-0046 Rev P)

+) Page 15 : "5 times with a delaybetween each of 500 milliseconds."

Thoughts : I am new to effective protocols to transmit data with the probability of collision - Just curious why there isn't a random element (eg 5 times with a a delay of 400 + random(100) ms ) - and a similar random delay before responding (ie 100ms). (100ms is an arbitrary choice in this example)

+) Page 18 : (keycode usage) "Send key code for Side Handle, user key D, press, followed by release. "
Suggestion : Change to "Send key code for pressing of Side Handle user key D (SH User D Press), followed by release."

+) Page 18: (keyaction usage)"Assume there is a preset loaded onthe camera called ISO10000."
Suggestion : Change to "Assume there is a preset file loaded onthe camera called ISO10000."
Reason: On my first pass reading this sentence, I though a (preset) key had been loaded on the camera called ISO10000.

+) Page 23: (Curve Parameter) "#$EXT:U:GCURVMY:10"
Suggestion : Either amend this to match the definition on page 20 ie "#$EXT:U:GCURVEMY:10" or amend the definition on Page 20 to match the example on page 23.

+) Page 23: (Lens Group) "Notice that F#, Fl, and Fd"

Thought 1 : I am not sure what these stand for when reading this sentence. Perhaps F# is a known shorthand for Aperture? My second thought was that 'F' might somehow be a precursor to the '#' start of message character (as that this the only mention of '#' so far in this document).

Thought 2 : Looking at the table - I deduce that 'F#' indicated Absolute Aperture - and that this has been depreciated, and that Aperture Target is now used.

AJ

 

[Antony Newman]

... continued.

Redlink Command Protocol Red Guide (910-0046 Rev P)

+) Page 23: (Lens Group) "FOCUSN - Far focal distance in meters*100"
Comment : I would guess needs to amended to "FOCUSN - Near focal distance in meters*100"

+) Page 24: (Lens Group) "PLDETECT " and "PLPOWER" don't have any any entries in the 'S', 'G', and 'C' columns.
Comment : Page 10 of this document implies that 'Command' characters are mandatory. I would Guess this means that there should be a 'S G C' command entry in the table for each of these?

+) Page 26: (Overlay Group) "SHCOLOR ... Look-around shading color. (see framgeguide_color_t)"
Comment : Typo : From rcp_types_public.h : This should be amended to 'frameguide_color_t'

+) Page 26: (Overlay Group) "GIOCPRI ... State 0 colors/labels for exposure zones (Gioscope) mode. "
Comment : The first TLA that came to mind for 'PRI' was Priority.
Suggest changing the description to "GIOCPRI ... State 0 (Primary) colors/labels for exposure zones (Gioscope) mode. "

+) Page 26: (Example of Changing Frame Guide Overlay) "#$EXT:H:F0MODEL:"
Comment : I believe this should be amended to "#$EXT:H:F0MODE:"

+) Page 28: (Examples of Setting Metadata) "REDCODE MSG_INT V G/H C/D"
Thought : I know this is depreciated - but was surprised that this was not an 'S'.

+) Page 29: (Examples of Setting Metadata) "CUSTMFPS CUSTMFMT CUSTMPHS CUSTMINT CUSTMISO CUSTMRC CUSTMAWB T"
Comment : The above 7 entries in the table refer to action 'T' that I guess was depreciated? (or never existed).
Comment : If it does exist - add this action into page 10 with ' T - Set parameter list values'

+) Page 30: (Examples of Setting Metadata) "VRISPDMD"
Comment : If you happen to be editing this document ... fix the fix the colour of the vertical separators around 'G' and 'C'.

+) Page 30: (Examples of Setting Metadata) "PRORESW .. Video codec image width in pixels (WEAPON and RAVEN only("
Comment : Typo. Change last bracket to ')'.

+) Page 30: (Examples of Creating Format2 Value Field) "Reserved (bit 17-31), set to 0"
Comment : The first time I read this I have in my mind that this is a 32 bit field and have to Scan the text - to find 4 lines below this that this comment is using zero indexed information. To avoid 'look around'
I would suggest putting 'Resolution (bits 0-3), see resolution_t type definition' at the top of this list.

AJ

 

[Antony Newman]

... continued.

Redlink Command Protocol Red Guide (910-0046 Rev P)

+) (State Group page 30) : Skimming this I noted that PREREC does not immediate opposite in the table to reverse the action - which then lead me to consider RECORD might used - but then noticed that set_record_state_t includes 'SET_RECORD_STATE_START_PRE_RECORD' as an enumerated type.

Which then got me wondering - which one of these is governing the pre-record 'state' - vs Starting to Record in the pre-record state. Perhaps this is obvious when you already know the information? I'll come back to this after I've understood the distinction. Perhaps I need a diagram?

+) Update.

If the PINK represent the state the camera is in ... in the absence of further information (elsewhere in this document, or perhaps in the SDK example code ... and without a state diagram ... I have to sort of guess how the pre-record words below: YELLOW represents the two guesses for (S) actions to change the state of the camera.

<RECORD_STATE_NOT_RECORDING>
RECORD (S) : SET_RECORD_STATE_START_PRE_RECORD or
PREREC (S) :
<RECORD_STATE_PRE_RECORDING>
<RECORD_STATE_RECORDING>
<RECORD_STATE_ENCODING>
<RECORD_STATE_FINALIZING>
<RECORD_STATE_NOT_RECORDING>

'will update this when information about the state changes is better understood.

+) Current Guess.

The 'RECORD' (S) actions will NOT Start recording when the camera is in a 'RECORD_STATE_PRE_RECORDING'.
When in this state - the 'PREREC' (S) action is used to start recording.

If this is correct - please update the Table 'RECORD MSG_INT S G C Start/Stop Recording. (See set_record_state_t/record_state_tfor parameters) '
to indicate that 'PREREC' must be used to actually start recording from 'RECORD_STATE_PRE_RECORDING'.

AJ

 

[Antony Newman]

... continued.

Redlink Command Protocol Red Guide (910-0046 Rev P)

+) Page 33: (System Group) "CLUTF1 ... Assign specified 3D LUT to Look B"
Comment : (Guess) Was this meant to be "Feed 1". Complete guess after finding a variable called 'RCP_PARAM_LUT_FEED_1' in rcp_api.h

+) Page 36: (System Group) "FANPCTF ... once per second. . On"
Comment : If you happen to be editing the document - remove the second '.'

+) Page 41: (Playback Group) "PLAY" and "PAUSE". "A value is required but it is ignored. "
Suggestion: If the intention is to enhance this in the future - suggest that a default value is used now - eg (0),
and that when this parameter is in use, non-zero values will have meaning.
Suggestion 2: If the intention is to remove this parameter advise - advise that Play&Pause will be depreciated, and created replacement parameters for Play and Pause. (so that code written now will not need altering)

AJ

 

[Trent Lillehaugen]

Thanks for the feedback, I will forward it along. I'll try to answer some of the more technical questions below.

+) Page 9 : "32 bit value with the major version in the high word and minor in the low" : I will assume that 'word' means 16bits for the RED SDK.

Yes, in this case we should specify a word is 16 bits.

+) Page 12 : "An example using the FLUT parameter is below. Its natural range is -8.0 to +8.0, the selected index is the160th being the last or 8.0 and can be specified in steps of 0.001. There are no prefix or postfix strings. #$SCARLET:FLUT:1|160|-8000|8000|100|1000|1|||: "
Comment : Reading this .. my first thought is that misread the definition of PRECISION / DIVIDER and whether what would be displayed to the user would be -8.0 or -8.000.
Perhaps an example of #$SCARLET:FLUT:1|159|-8000|8000|100|1000|1|||: would make this more clear? With a value of +7.999

The ambiguity comes from the definition of 'PRECISION' in the sentence 'PRECISION: the smallest natural value increment. With a DIVIDER of 1000 and PRECISION of 1, the natural value is represented to 0.001. ". I was not sure if this sentence was telling me Log(Divider)/Log(10) = 3 .. and there the value presented to the user should have 3 significant figures ... OR that you 3 decimal places were desired. +8.000 would have been what I was expecting .. but +8.0 is what was listed in the example.

When defining a list in this manner, we use:
min: minimum integer value of parameter
max: maximum integer value of parameter
step: integer step size to create each entry in the list: min, min + step, min + (2 * step), ..., max
divider: used to compute floating point value from integer representation. float = integer / divider
precision: the number of digits to display to the user


This is the code we use in c_list_fill_generic (found in: c_list.c):


for (ii = min; ii <= max; ii += step)
{
/* ... */
snprintf(num, sizeof(num), "%.*f", precision, (float) ii / divider);
}

Redlink Command Protocol Red Guide (910-0046 Rev P)

+) Page 15 : "5 times with a delaybetween each of 500 milliseconds."

Thoughts : I am new to effective protocols to transmit data with the probability of collision - Just curious why there isn't a random element (eg 5 times with a a delay of 400 + random(100) ms ) - and a similar random delay before responding (ie 100ms). (100ms is an arbitrary choice in this example)

This was just one example of how you could implement a camera discovery scheme. However, the collisions in this case are going to come from multiple cameras responding to your G:CAMINFO packets, not multiple applications sending out G:CAMINFO packets at the same time. So the randomized delay response is handled in the camera firmware.

+) Page 23: (Lens Group) "FOCUSN - Far focal distance in meters*100"
Comment : I would guess needs to amended to "FOCUSN - Near focal distance in meters*100"

Yes, FOCUSN should be the NEAR focal distance.

+) Page 24: (Lens Group) "PLDETECT " and "PLPOWER" don't have any any entries in the 'S', 'G', and 'C' columns.
Comment : Page 10 of this document implies that 'Command' characters are mandatory. I would Guess this means that there should be a 'S G C' command entry in the table for each of these?

Yes, PLDETECT and PLPOWER each have 'S', 'G', and 'C'.

+) Page 30: (Examples of Creating Format2 Value Field) "Reserved (bit 17-31), set to 0"
Comment : The first time I read this I have in my mind that this is a 32 bit field and have to Scan the text - to find 4 lines below this that this comment is using zero indexed information. To avoid 'look around'
I would suggest putting 'Resolution (bits 0-3), see resolution_t type definition' at the top of this list.

I'm not sure I understand your confusion. FORMAT2 is a bitfield. We define what each range of bits represents. The top bits (bits 17-31) are reserved (we don't use them at the moment), and should always be set to 0.

+) (State Group page 30) : Skimming this I noted that PREREC does not immediate opposite in the table to reverse the action - which then lead me to consider RECORD might used - but then noticed that set_record_state_t includes 'SET_RECORD_STATE_START_PRE_RECORD' as an enumerated type.

Which then got me wondering - which one of these is governing the pre-record 'state' - vs Starting to Record in the pre-record state. Perhaps this is obvious when you already know the information? I'll come back to this after I've understood the distinction. Perhaps I need a diagram?

+) Update.

If the PINK represent the state the camera is in ... in the absence of further information (elsewhere in this document, or perhaps in the SDK example code ... and without a state diagram ... I have to sort of guess how the pre-record words below: YELLOW represents the two guesses for (S) actions to change the state of the camera.

<RECORD_STATE_NOT_RECORDING>
RECORD (S) : SET_RECORD_STATE_START_PRE_RECORD or
PREREC (S) :
<RECORD_STATE_PRE_RECORDING>
<RECORD_STATE_RECORDING>
<RECORD_STATE_ENCODING>
<RECORD_STATE_FINALIZING>
<RECORD_STATE_NOT_RECORDING>

'will update this when information about the state changes is better understood.

+) Current Guess.

The 'RECORD' (S) actions will NOT Start recording when the camera is in a 'RECORD_STATE_PRE_RECORDING'.
When in this state - the 'PREREC' (S) action is used to start recording.

If this is correct - please update the Table 'RECORD MSG_INT S G C Start/Stop Recording. (See set_record_state_t/record_state_tfor parameters) '
to indicate that 'PREREC' must be used to actually start recording from 'RECORD_STATE_PRE_RECORDING'.

S:RECORD:SET_RECORD_STATE_START_PRE_RECORD and SREREC are equivalent. Once in RECORD_STATE_PRE_RECORDING you can start recording by either S:RECORD:SET_RECORD_STATE_START or S:RECORD:SET_RECORD_STATE_TOGGLE

+) Page 33: (System Group) "CLUTF1 ... Assign specified 3D LUT to Look B"
Comment : (Guess) Was this meant to be "Feed 1". Complete guess after finding a variable called 'RCP_PARAM_LUT_FEED_1' in rcp_api.h

The RCP parameters and parameter names are often created before the final UI wording is chosen (since the camera UI itself uses the API). In some cases we go back and rename the parameters, in other times we either forget, don't have time, or have already released a version of the API with the old names, etc. This is one of those times.

Namely, what is now called LOOK A/B used to be called FEED 0/1.

Trent

 

[Antony Newman]

Thanks for the insight Trent,

Hope my 'stream of consciousness' in this thread was not too anal...

One last comment on this doc:

+) Page 47: (Output Group) "MONPRTY T'
Comment : Action 'T' not defined on page 10.

AJ

 

[Mikael Lubtchansky]

Thanks for the insight Trent,

Hope my 'stream of consciousness' in this thread was not too anal...

One last comment on this doc:

+) Page 47: (Output Group) "MONPRTY T'
Comment : Action 'T' not defined on page 10.

AJ

T FOR MONPRTY is the rare specie of a Set for lists
(Like D is a Current for lists and H is a Get)

 

[Antony Newman]

Thanks Mikael.

AJ

 

[Antony Newman]

Just started reading the REDLINK API DEVELOPMENT GUIDE - Revision F - and will list my observations here.

+) This document encapsulates the Redlink Command Protocol Red Guide - for others who also intend to write software for this ecosystem ... start with this document first!
(Maybe the objective of the RCP should direct people this way first?)

AJ

 

[Trent Lillehaugen]

Just started reading the REDLINK API DEVELOPMENT GUIDE - Revision F - and will list my observations here.

+) This document encapsulates the Redlink Command Protocol Red Guide - for others who also intend to write software for this ecosystem ... start with this document first!
(Maybe the objective of the RCP should direct people this way first?)

I would also like to add that we don't recommend anybody use RCP without using the API. Unless it is for a VERY SIMPLE application - like an underwater housing that just sends key codes. That being said, understanding the underlying protocol can help understand the API.

 

[Antony Newman]

Thanks Trent.

REDLINK API DEVELOPMENT GUIDE - Revision F

+) Page 12 : "This can be useful in an about page of the application."
(minor) Suggestion : Change to "This can be useful in an informational page within the application."

+) Page 15 : (10th line of table) "has_set_uint Int If true, calls to rcp_set_uint_relative() are valid for this parameter "
Comment : 'has_set_uint' should be 'has_set_uint_relative'

+) Page 16 : (Display strings) : "These are provided in the structure returned by most of the cur_<type>_cb() callbacks."
Comment : Suggest amending to "These are provided in the structure returned by most of the cur_<type>_cb() callbacks (in rcp_camera_connection_info_t struct)"
Reason 1 : At this stage - the reader might be trying to search for an expression to see what this relates and may not know the code well enough.
Reason 2 : "Two member variables of the return data structure" refers to the typdef that (eg) 'cur_list_cb' refers to (eg 'rcp_cur_list_cb_data_t').
Gut feeling is that without being able to see the code fragment in front of them, and with two levels of abstraction .... people are less likely to have the 'aha' moment.

+) Page 21: (composite parameters) "There are six composite parameters shown in the table below"
Comment : 'six' should be 'twelve'
Comment : (right side of table / bottom 5 items) : Remove commas after component list - to match all items above them in the table.

+) 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

AJ

 

[Antony Newman]

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

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."

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

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

AJ

 

[Antony Newman]

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

 

[Antony Newman]

REDLINK SDK - continued.

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.


rcp_parser2.h

+) '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).

+) 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++).

+) "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.


test_RCP_get_and_parse_packet2.c

+) 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.

AJ

 

[Antony Newman]

REDLINK SDK - continued.

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.


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...


AJ

 

[Antony Newman]

REDLINK SDK - continued.

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_...


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)
{
...
}

AJ

 

[Antony Newman]

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