Address review comments

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

rosidl_generator_c/rosidl_generator_c/__init__.py

@@ -14,6 +14,8 @@
 
 import re
 
+from rosidl_generator_type_description import parse_rihs_string
+from rosidl_generator_type_description import RIHS01_HASH_VALUE_SIZE
 from rosidl_parser.definition import AbstractGenericString
 from rosidl_parser.definition import AbstractSequence
 from rosidl_parser.definition import AbstractString
@@ -225,18 +227,14 @@ def escape_wstring(s):
 
 def type_hash_to_c_definition(variable_name, hash_string, indent=0):
     """Generate empy for rosidl_type_hash_t instance with 8 bytes per line for readability."""
-    hash_length = 32
     bytes_per_line = 8
 
     indent_str = ' ' * (indent + 2)
-    pattern = re.compile(r'RIHS([0-9a-f]{2})_([0-9a-f]{64})')
-    match = pattern.match(hash_string)
-    if not match:
-        raise Exception('Type hash string does not match expected RIHS format')
-    version, value = match.group(1, 2)
+    version, value = parse_rihs_string(hash_string)
+    assert version == 1, 'This function only knows how to generate RIHS01 definitions.'
 
     result = f'rosidl_type_hash_t {variable_name} = {{{version}, {{'
-    for i in range(hash_length):
+    for i in range(RIHS01_HASH_VALUE_SIZE):
         if i % bytes_per_line == 0:
             result += f'\n{indent_str}  '
         result += f'0x{value[i * 2:i * 2 + 2]},'

rosidl_generator_type_description/resource/HashedTypeDescription.schema.json

@@ -2,7 +2,7 @@
   "$schema": "https://json-schema.org/draft/2020-12/schema",
   "$id": "HashedTypeDescription.schema.json",
   "title": "HashedTypeDescription",
-  "description": "TODO",
+  "description": "Contains hashes and full type description for a ROS 2 interface. TypeDescription, IndividualTypeDescription, Field, and FieldType are exact representations of type_description_interfaces/msg types, see their .msg files for semantic comments.",
   "type": "object",
   "properties": {
     "hashes": {
@@ -66,11 +66,11 @@
     },
     "TypeDescription": {
       "type": "object",
-      "$comment": "All whitespace must be excluded for consistent hashing, which this schema cannot enforce.",
+      "$comment": "For hashing: All whitespace must be excluded, which this schema cannot enforce.",
       "properties": {
         "type_description": {"$ref": "#/$defs/IndividualTypeDescription"},
         "referenced_type_descriptions": {
-          "$comment": "Referenced type descriptions must be alphabetized, which this schema cannot enforce.",
+          "$comment": "For hashing: Referenced type descriptions must be alphabetized, which this schema cannot enforce.",
           "type": "array",
           "items": { "$ref": "#/$defs/IndividualTypeDescription" }
         }

rosidl_generator_type_description/rosidl_generator_type_description/__init__.py

@@ -15,14 +15,21 @@
 import hashlib
 import json
 from pathlib import Path
+import re
 import sys
 from typing import List, Tuple
 
 from rosidl_parser import definition
 from rosidl_parser.parser import parse_idl_file
 
-# ROS Interface Hashing Standard, per REP-2011
-RIHS_VERSION = '01'
+# RIHS: ROS Interface Hashing Standard, per REP-2011
+# NOTE: These values and implementations must be updated if
+# - type_description_interfaces messsages change, or
+# - the hashing algorithm for type descriptions changes
+# Both changes require an increment of the RIHS version
+RIHS01_PREFIX = 'RIHS01_'
+RIHS01_HASH_VALUE_SIZE = 32
+RIHS01_PATTERN = re.compile(r'RIHS([0-9a-f]{2})_([0-9a-f]{64})')
 
 
 def generate_type_hash(generator_arguments_file: str) -> List[str]:
@@ -72,6 +79,15 @@ def generate_type_hash(generator_arguments_file: str) -> List[str]:
     return generated_files
 
 
+def parse_rihs_string(rihs_str: str) -> Tuple[int, str]:
+    """Parse RIHS string, return (version, value) tuple."""
+    match = RIHS01_PATTERN.match(rihs_str)
+    if not match:
+        raise ValueError(f'Type hash string {rihs_str} does not match expected RIHS format.')
+    version, value = match.group(1, 2)
+    return (int(version), value)
+
+
 # This mapping must match the constants defined in type_description_interfaces/msgs/FieldType.msg
 # NOTE: Nonexplicit integer types are not defined in FieldType (short, long, long long).
 # If a ROS IDL uses these, this generator will throw a KeyError.
@@ -446,10 +462,9 @@ def _calculate_hash_tree(self) -> dict:
             separators=(',', ': '),
             sort_keys=False
         )
-        prefix = f'RIHS{RIHS_VERSION}_'
         sha = hashlib.sha256()
         sha.update(hashable_repr.encode('utf-8'))
-        type_hash = prefix + sha.hexdigest()
+        type_hash = RIHS01_PREFIX + sha.hexdigest()
 
         type_hash_infos = {
             self.interface_type: type_hash,

rosidl_runtime_c/CMakeLists.txt

@@ -30,7 +30,6 @@ target_include_directories(${PROJECT_NAME} PUBLIC
 ament_target_dependencies(${PROJECT_NAME}
   "rcutils"
   "rosidl_typesupport_interface")
-target_link_libraries(${PROJECT_NAME} m)
 if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID MATCHES "Clang")
   set_target_properties(${PROJECT_NAME} PROPERTIES
     COMPILE_OPTIONS -Wall -Wextra -Wpedantic)

rosidl_runtime_c/include/rosidl_runtime_c/type_hash.h

@@ -68,6 +68,8 @@ rosidl_stringify_type_hash(
 /**
  * \param[in] type_hash_string Null-terminated string with the hash representation
  * \param[out] hash_out Preallocated structure to be filled with parsed hash information.
+ *   hash_out->version will be 0 if no version could be parsed,
+ *   but if a version could be determined this field will be set even if an error is returned
  * \return RCTUILS_RET_INVALID_ARGUMENT on any null pointer argumunts, or malformed hash string.
  * \return RCUTILS_RET_OK otherwise
  */

rosidl_runtime_c/src/type_hash.c

@@ -12,12 +12,51 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include <math.h>
+#include <string.h>
 
 #include "rosidl_runtime_c/type_hash.h"
 
 #include "rcutils/error_handling.h"
-#include "rcutils/format_string.h"
+
+static const char RIHS01_PREFIX[] = "RIHS01_";
+// Hash representation is hex string, two characters per byte
+static const size_t RIHS_VERSION_IDX = 4;
+static const size_t RIHS_PREFIX_LEN = 7;
+static const size_t RIHS01_STRING_LEN = RIHS_PREFIX_LEN + (ROSIDL_TYPE_HASH_SIZE * 2);
+
+static bool _ishexdigit(char c)
+{
+  return (c >= '0' && c <= '9') || (c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f');
+}
+
+/// Translate a single character hex digit to a nibble
+static uint8_t _xatoi(char c)
+{
+  if (c >= '0' && c <= '9') {
+    return c - '0';
+  }
+  if (c >= 'A' && c <= 'F') {
+    return c - 'A' + 0xa;
+  }
+  if (c >= 'a' && c <= 'f') {
+    return c - 'a' + 0xa;
+  }
+  return -1;
+}
+
+/// Tranlate a byte value to two hex characters
+static void _xitoa(uint8_t val, char * dest)
+{
+  uint8_t nibble = 0;
+  for (size_t n = 0; n < 2; n++) {
+    nibble = (val >> (4 * n)) & 0x0f;
+    if (nibble < 0xa) {
+      dest[n] = '0' + nibble;
+    } else {  // 0xa <= nibble < 0x10
+      dest[n] = 'a' + (nibble - 0xa);
+    }
+  }
+}
 
 rosidl_type_hash_t
 rosidl_get_zero_initialized_type_hash(void)
@@ -32,78 +71,76 @@ rosidl_stringify_type_hash(
   rcutils_allocator_t allocator,
   char ** output_string)
 {
-  if (!type_hash) {
-    RCUTILS_SET_ERROR_MSG("Null type_hash argument.");
-    return RCUTILS_RET_INVALID_ARGUMENT;
-  }
+  RCUTILS_CHECK_ARGUMENT_FOR_NULL(type_hash, RCUTILS_RET_INVALID_ARGUMENT);
   if (!rcutils_allocator_is_valid(&allocator)) {
     RCUTILS_SET_ERROR_MSG("Invalid allocator");
     return RCUTILS_RET_INVALID_ARGUMENT;
   }
-  if (!output_string) {
-    RCUTILS_SET_ERROR_MSG("Null string destination pointer.");
-    return RCUTILS_RET_INVALID_ARGUMENT;
-  }
+  RCUTILS_CHECK_ARGUMENT_FOR_NULL(output_string, RCUTILS_RET_INVALID_ARGUMENT);
 
-  // Hash representation is hex string, two characters per byte
-  const char * fmt = "RIHS%02d_%64d";
-  const size_t prefix_len = strlen("RIHS01_");
-  char * local_output = rcutils_format_string(allocator, fmt, type_hash->version, 0);
+  char * local_output = allocator.allocate(RIHS01_STRING_LEN + 1, allocator.state);
   if (!local_output) {
     *output_string = NULL;
-    RCUTILS_SET_ERROR_MSG("Unable to allocate space for stringified type hash.");
+    RCUTILS_SET_ERROR_MSG("Unable to allocate space for type hash string.");
     return RCUTILS_RET_BAD_ALLOC;
   }
+  local_output[RIHS01_STRING_LEN] = '\0';
+  memcpy(local_output, RIHS01_PREFIX, RIHS_PREFIX_LEN);
   for (size_t i = 0; i < ROSIDL_TYPE_HASH_SIZE; i++) {
-    snprintf(local_output + prefix_len + (i * 2), 3, "%02x", type_hash->value[i]);  // NOLINT
+    _xitoa(type_hash->value[i], local_output + RIHS_PREFIX_LEN + (i * 2));
   }
 
   *output_string = local_output;
   return RCUTILS_RET_OK;
 }
 
-static int _xatoi(char c)
-{
-  if (c >= '0' && c <= '9') {
-    return c - '0';
-  }
-  if (c >= 'A' && c <= 'F') {
-    return c - 'A' + 0xa;
-  }
-  if (c >= 'a' && c <= 'f') {
-    return c - 'a' + 0xa;
-  }
-  return -1;
-}
-
-static int _str_to_byte(const char * str)
-{
-  return (_xatoi(str[0]) << 4) + _xatoi(str[1]);
-}
-
 rcutils_ret_t
 rosidl_parse_type_hash_string(
   const char * type_hash_string,
   rosidl_type_hash_t * hash_out)
 {
   RCUTILS_CHECK_ARGUMENT_FOR_NULL(type_hash_string, RCUTILS_RET_INVALID_ARGUMENT);
   RCUTILS_CHECK_ARGUMENT_FOR_NULL(hash_out, RCUTILS_RET_INVALID_ARGUMENT);
-  static const size_t kprefix_len = sizeof("RIHS01_") - 1;
-  static const size_t kvalue_len = 64;
   hash_out->version = 0;
+  size_t input_len = strlen(type_hash_string);
 
-  if (strlen(type_hash_string) != (kprefix_len + kvalue_len)) {
-    RCUTILS_SET_ERROR_MSG("Hash string incorrect size.");
+  // Check prefix
+  if (input_len < RIHS_PREFIX_LEN) {
+    RCUTILS_SET_ERROR_MSG("Hash string not long enough to contain RIHS prefix.");
     return RCUTILS_RET_INVALID_ARGUMENT;
   }
-  if (0 != strncmp(type_hash_string, "RIHS01_", 7)) {
-    RCUTILS_SET_ERROR_MSG("Type hash string is not prefixed RIHS01_");
+  if (0 != strncmp(type_hash_string, RIHS01_PREFIX, RIHS_VERSION_IDX)) {
+    RCUTILS_SET_ERROR_MSG("Hash string doesn't start with RIHS.");
     return RCUTILS_RET_INVALID_ARGUMENT;
   }
-  hash_out->version = 1;
-  const char * value_str = type_hash_string + kprefix_len;
-  for (size_t i = 0; i < 32; i++) {
-    int byte_val = _str_to_byte(value_str + (i * 2));
+
+  // Parse version
+  char version_top = type_hash_string[RIHS_VERSION_IDX];
+  char version_bot = type_hash_string[RIHS_VERSION_IDX + 1];
+  if (!(_ishexdigit(version_top) && _ishexdigit(version_bot))) {
+    RCUTILS_SET_ERROR_MSG("RIHS version is not a 2-digit hex string.");
+    return RCUTILS_RET_INVALID_ARGUMENT;
+  }
+  hash_out->version = (_xatoi(version_top) << 4) + _xatoi(version_bot);
+
+  if (hash_out->version != 1) {
+    RCUTILS_SET_ERROR_MSG("Do not know how to parse RIHS version.");
+    return RCUTILS_RET_INVALID_ARGUMENT;
+  }
+  if (input_len != RIHS01_STRING_LEN) {
+    RCUTILS_SET_ERROR_MSG("RIHS string is the incorrect size to contain a RIHS01 value.");
+    return RCUTILS_RET_INVALID_ARGUMENT;
+  }
+  const char * value_str = type_hash_string + RIHS_PREFIX_LEN;
+  for (size_t i = 0; i < ROSIDL_TYPE_HASH_SIZE; i++) {
+    if (!_ishexdigit(value_str[i * 2])) {
+      RCUTILS_SET_ERROR_MSG("Type hash string value contained non-hex-digit character.");
+      return RCUTILS_RET_INVALID_ARGUMENT;
+    }
+  }
+  for (size_t i = 0; i < ROSIDL_TYPE_HASH_SIZE; i++) {
+    // No error checking on byte values because the whole string was checked in prior loop
+    uint8_t byte_val = (_xatoi(value_str[i * 2]) << 4) + _xatoi(value_str[i * 2 + 1]);
     if (byte_val < 0) {
       RCUTILS_SET_ERROR_MSG("Type hash string value did not contain only hex digits.");
       return RCUTILS_RET_INVALID_ARGUMENT;

rosidl_runtime_c/test/test_type_hash.cpp

@@ -92,5 +92,6 @@ TEST(type_hash, parse_bad_version) {
     "RIHS02_00112233445566778899aabbccddeeff00112233445566778899aabbccddeeff";
   rosidl_type_hash_t hash = rosidl_get_zero_initialized_type_hash();
   EXPECT_EQ(RCUTILS_RET_INVALID_ARGUMENT, rosidl_parse_type_hash_string(test_value.c_str(), &hash));
+  EXPECT_EQ(hash.version, 2);
   rcutils_reset_error();
 }