Repository: ebroecker/canmatrix

Issue:
Cannot parse "Char", "String" or "Raw" signal types in .Sym files
The PCAN Symbol editor allows several types to be set for signals. These are:

- Bit 
- Char
- String
- Signed
- Unsigned
- Float
- Enum
- Double 
- Raw


Currently Canmatrix will accept some of these but not all. 
The ones which do not work yet are:

- Char
- String
- Enum
- Raw

I think that the first thing to do is to make it interpret all of these except Enum as unsigned. So that it doesn't throw any signals away.
I think it would also be useful to add a property to indicate what type the symbol file specified the signal to be, so that a symbol file can be written back with the same types as it was read with.

(Adding Enums is a bit more complex, so ill open a separate issue for those.)

Discussion:
Seneda: @ebroecker Do you see any problems with how I've implemented this? I'm happy to make any changes if you don't think this should be merged as it is.

Patch:
diff --git a/src/canmatrix/canmatrix.py b/src/canmatrix/canmatrix.py
index 9a5dd66..bb21b55 100644
--- a/src/canmatrix/canmatrix.py
+++ b/src/canmatrix/canmatrix.py
@@ -156,6 +156,8 @@ class Signal(object):
 
     mux_value = attr.ib(default=None)
     is_float = attr.ib(default=False)  # type: bool
+    is_ascii = attr.ib(default=False)  # type: bool
+    type_label = attr.ib(default="")
     enumeration = attr.ib(default=None)  # type: typing.Optional[str]
     comments = attr.ib(factory=dict)  # type: typing.MutableMapping[int, str]
     attributes = attr.ib(factory=dict)  # type: typing.MutableMapping[str, typing.Any]
diff --git a/src/canmatrix/formats/sym.py b/src/canmatrix/formats/sym.py
index a67d137..7567864 100644
--- a/src/canmatrix/formats/sym.py
+++ b/src/canmatrix/formats/sym.py
@@ -104,11 +104,25 @@ def create_signal(db, signal):  # type: (canmatrix.CanMatrix, canmatrix.Signal)
     global enums
     global enum_dict
     output = ""
-    output += "Var=%s " % signal.name
-    if not signal.is_signed:
-        output += "unsigned "
+    if sys.version_info > (3, 0):
+        quote_name = not signal.name.isidentifier()
     else:
-        output += "signed "
+        from future.utils import isidentifier
+        quote_name = not isidentifier(signal.name)
+    if quote_name:
+        output += 'Var="%s" ' % signal.name
+    else:
+        output += "Var=%s " % signal.name
+    if signal.type_label:
+        output += signal.type_label + " "
+    else:
+        if signal.is_signed:
+            output += "signed "
+        elif signal.is_float:
+            output += "float "
+        else:
+            output += "unsigned "
+
     start_bit = signal.get_startbit()
     if not signal.is_little_endian:
         # Motorola
@@ -419,29 +433,29 @@ def load(f, **options):  # type: (typing.IO, **typing.Any) -> canmatrix.CanMatri
                     sig_name = temp_array[0]
 
                     is_float = False
+                    is_ascii = False
                     if index_offset != 1:
                         is_signed = True
                     else:
                         is_signed = False
 
-                        if temp_array[1] == 'unsigned':
+                        type_label = temp_array[1]
+
+                        if type_label == 'unsigned':
+                            pass
+                        elif type_label == 'bit':
                             pass
-                        elif temp_array[1] == 'bit':
-                            # TODO: actually support bit instead of interpreting as
-                            # an unsigned
+                        elif type_label == 'raw':
                             pass
-                        elif temp_array[1] == 'signed':
+                        elif type_label == 'signed':
                             is_signed = True
-                        elif temp_array[1] in ['float', 'double']:
+                        elif type_label in ['float', 'double']:
                             is_float = True
-                        elif temp_array[1] in ['string']:
-                            # TODO: actually support these variable types instead
-                            # of skipping
-                            print('Variable type \'{}\' found and skipped'
-                                  .format(temp_array[1]))
-                            continue
+                        elif type_label in ['char', 'string']:
+                            is_ascii = True
+                            pass
                         else:
-                            raise ValueError('Unknown type \'{}\' found'.format(temp_array[1]))
+                            raise ValueError('Unknown type \'{}\' found'.format(type_label))
 
                     start_bit = int(temp_array[index_offset + 1].split(',')[0])
                     signal_length = int(temp_array[index_offset + 1].split(',')[1])
@@ -521,6 +535,7 @@ def load(f, **options):  # type: (typing.IO, **typing.Any) -> canmatrix.CanMatri
                                 is_little_endian=intel,
                                 is_signed=is_signed,
                                 is_float=is_float,
+                                is_ascii=is_ascii,
                                 factor=factor,
                                 offset=offset,
                                 unit=unit,
@@ -560,6 +575,7 @@ def load(f, **options):  # type: (typing.IO, **typing.Any) -> canmatrix.CanMatri
                             unit=unit,
                             multiplex=multiplexor,
                             comment=comment,
+                            type_label=type_label,
                             **extras)
                         if min_value is not None:
                             signal.min = float_factory(min_value)


Test patch:
diff --git a/src/canmatrix/tests/test_sym.py b/src/canmatrix/tests/test_sym.py
index d5b5ccd..cb99d69 100644
--- a/src/canmatrix/tests/test_sym.py
+++ b/src/canmatrix/tests/test_sym.py
@@ -2,6 +2,7 @@
 import io
 import sys
 import textwrap
+from itertools import chain
 
 import pytest
 
@@ -218,3 +219,101 @@ Title="An Example Title"
         assert matrix.value_tables == enum_dict, "Enum not parsed correctly : '{}'".format(enum_label)
         f_out = io.BytesIO()
         canmatrix.formats.sym.dump(matrix, f_out)
+
+
+def test_types_read():
+    f = io.BytesIO('''\
+FormatVersion=5.0 // Do not edit this line!
+Title="Types Test"
+
+{ENUMS}
+enum EnumAnimals(0="Cat", // An enum value for cats
+  1="Dog", // An enum value for dogs
+  2="Horse", 3="Monkey", 
+  4="Lion")// An enum with a comment for the final value
+
+{SENDRECEIVE}
+
+[SymbolLengths]
+ID=000h
+DLC=8
+Var="1Bit" unsigned 0,1
+Var="3Bits" unsigned 1,3
+Var="4Bits" unsigned 4,4
+Var="21Bits" unsigned 8,21
+Var="6Bits" unsigned 29,6
+Var="29Bits" unsigned 35,29
+
+[SymbolTypes]
+ID=001h
+DLC=8
+Var=Bit bit 0,1
+Var=Char char 1,8
+Var=String string 16,16
+Var=Signed signed 32,4
+Var=Unsigned unsigned 36,4
+'''  # Var=Enum EnumAnimals 40,4
+                   '''
+Var=Raw raw 48,16
+
+[SymbolDouble]
+ID=002h
+DLC=8
+Var=Double double 0,64	// Must be 8 Bytes according to PCAN Symbol Editor V5
+
+[SymbolFloat]
+ID=003h
+DLC=4
+Var=Float float 0,32	// Must be 4 Bytes according to PCAN Symbol Editor V5
+'''.encode('utf-8'),
+                   )
+    matrix = canmatrix.formats.sym.load(f)
+    # Check no errors loading the matrix
+    assert matrix.load_errors == []
+
+    f_out = io.BytesIO()
+    canmatrix.formats.sym.dump(matrix, f_out)
+    f_out_bytes = f_out.getvalue()
+    f_out_string = f_out_bytes.decode("utf-8")
+
+    # Check that types are preserved when saving back to .SYM format
+    assert "Var=Bit bit" in f_out_string
+    assert "Var=Char char" in f_out_string
+    assert "Var=String string" in f_out_string
+    assert "Var=Signed signed" in f_out_string
+    assert 'Var="21Bits" unsigned' in f_out_string
+    assert 'Var=Float float' in f_out_string
+    assert 'Var=Double double' in f_out_string
+
+    # Read matrix back in to check all symbols/frames preserved
+    f_in = io.BytesIO(f_out_bytes)
+    new_matrix = canmatrix.formats.sym.load(f_in)
+
+    # Check no errors loading the matrix
+    assert new_matrix.load_errors == []
+
+    # Check that both matrices have the same Frames
+    frames = [f.name for f in matrix.frames]
+    new_frames = [f.name for f in new_matrix.frames]
+    assert sorted(frames) == sorted(new_frames)
+
+    # Check that both matrices have the same signals, and that all the expected signals are present
+    signals = chain(*[[s.name for s in frame.signals] for frame in matrix.frames])
+    new_signals = chain(*[[s.name for s in frame.signals] for frame in new_matrix.frames])
+    assert sorted(signals) == sorted(new_signals) == sorted([
+                                                            "1Bit",
+                                                            "3Bits",
+                                                            "4Bits",
+                                                            "21Bits",
+                                                            "6Bits",
+                                                            "29Bits",
+                                                            "Bit",
+                                                            "Char",
+                                                            "String",
+                                                            "Signed",
+                                                            "Unsigned",
+                                                            "Raw",
+                                                            "Double",
+                                                            "Float", ])
+
+