[PATCH] Fix debugging output in MultiwiiConf GUI

[Tommie]

I noticed that with the current Multiwiiconf GUI, the debug variables do not work, and neither does e.g. MAG calibration. I tracked down the issue and identified the misue of strings and char() casts s the source. Java's char is not the same as C's (aka uint8_t), for every value above 127, UTF8 magic is applied, breaking the request message.

I fixed the debug output with this changeset: https://github.com/wertarbyte/multiwii- ... /fix_debug

[kos]

erf ..

edit 1 : processing does not use enum

Code: Select all

public enum MSP {
   MSP_GS_TO_FC( "$M<"), 
   IDENT((char)21), 
   
   STATUS               ((char)101),
   RAW_IMU               ((char)102),
   SERVO                ((char)103),
   MOTOR                ((char)104),
   RC                   ((char)105),
   RAW_GPS               ((char)106),
   COMP_GPS             ((char)107),
   ATTITUDE             ((char)108),
   ALTITUDE             ((char)109),
   BAT                  ((char)110),
   RC_TUNING            ((char)111),
   PID                  ((char)112),
   BOX                  ((char)113),
   MISC                 ((char)114),

   SET_RAW_RC           ((char)200),
   SET_RAW_GPS          ((char)201),
   SET_PID               ((char)202),
   SET_BOX               ((char)203),
   SET_RC_TUNING        ((char)204),
   ACC_CALIBRATION      ((char)205),
   MAG_CALIBRATION      ((char)206),
   SET_MISC             ((char)207),
   RESET_CONF           ((char)208),

   EEPROM_WRITE         ((char)250),

   DEBUG                ((byte)254);
    
    private Object code;
    
    private MSP(Object c) {
      code = c;
    }
    
    public String toString() {
      return code;
    }
}

[Hamburger]

from a quick glance I do not understand
- why that diff (splitting commands into two sequences) could fix error,
- what is the importance of the enum posting in that context?
I am just curious, not doubting your expertise. Care to explain, please?

[kos]

-splitting does not change anything the order of the cmd in the output buffer does not matter . .. it is just for having char and byte casting on different line for clarity

- an enum would have provide the right object without casting

if we output ATTITUDE we want to write ((char)108)
if we output DEBUG we want to write ((byte)254);

[Tommie]

from a quick glance I do not understand
- why that diff (splitting commands into two sequences) could fix error,
- what is the importance of the enum posting in that context?
I am just curious, not doubting your expertise. Care to explain, please?

Please read the commit text. You cannot simply put integer (or the equivalent of uint8_t) values in a Java string and expect the same bits to fall out; this only works for values in the ASCII range, since they are identical with their UTF8 counterparts. This explains while the GUI in general seems to work, but e.g. calibration of the magnetometer and the debugging output fails: those message codes are above 127, casting them into a string probably triggers UTF8 conversion, turning the command incomprehensible to the MWC firmware.

https://github.com/wertarbyte/multiwii- ... e252d1acff

[Tommie]

if we output ATTITUDE we want to write ((char)108)
if we output DEBUG we want to write ((byte)254);

No.
You should never write "char" at all, since char is a 16 bit data type. It only works "by accident", since the multiwii code will ignore the second byte. It probably however fails for anything that is not in the ASCII range.

[kos]

not by accident ..

the code only take care of the bottom 8 bits

Code: Select all

 /**
   * Write a String to the output. Note that this doesn't account
   * for Unicode (two bytes per char), nor will it send UTF8 
   * characters.. It assumes that you mean to send a byte buffer 
   * (most often the case for networking and serial i/o) and
   * will only use the bottom 8 bits of each char in the string.
   * (Meaning that internally it uses String.getBytes)
   *
   * If you want to move Unicode data, you can first convert the
   * String to a byte stream in the representation of your choice
   * (i.e. UTF8 or two-byte Unicode data), and send it as a byte array.
   */
  public void write(String what) {
    write(what.getBytes());
  }

Code: Select all

  /**
   * This will handle both ints, bytes and chars transparently.
   */
  public void write(int what) {  // will also cover char
    try {
      output.write(what & 0xff);  // for good measure do the &
      output.flush();   // hmm, not sure if a good idea

    } catch (Exception e) { // null pointer or serial port dead
      errorMessage("write", e);
    }
  }

[Tommie]

not by accident ..

the code only take care of the bottom 8 bits

And is broken for half of it. Great, so why not simply use "byte" instead of char? Obviously, aggregating arbitrary bytes in a String does not work.

[Hamburger]

from a quick glance I do not understand
- why that diff (splitting commands into two sequences) could fix error,
- what is the importance of the enum posting in that context?
I am just curious, not doubting your expertise. Care to explain, please?

Please read the commit text. You cannot simply put integer (or the equivalent of uint8_t) values in a Java string and expect the same bits to fall out; this only works for values in the ASCII range, since they are identical with their UTF8 counterparts. This explains while the GUI in general seems to work, but e.g. calibration of the magnetometer and the debugging output fails: those message codes are above 127, casting them into a string probably triggers UTF8 conversion, turning the command incomprehensible to the MWC firmware.

ok, thanks. I had not seen the replacement of char to byte for the DEBUG message.

[Tommie]

not by accident ..

the code only take care of the bottom 8 bits

And these bottom 8 bits are not the same put into the string. Here is a small test case. Please try to guess the output first, then compile and run:

Code: Select all

class Chartest {
   public static void main(String[] args) {
      byte entry = (byte)0xFE; // 254 (unsigned)
      String foo = "a" + "b" + (char)entry;
      byte[] b = foo.getBytes();
      for (int i=0; i<b.length; i++) {
         int unsigned = b[i] & 0xff; // convert to unsigned number
         System.out.println(unsigned);
      }
   }
}

[kos]

so lets write to an bytes[] and then call write(byte bytes[]) ?

http://www.processing.org/reference/lib ... rite_.html

byte: The byte data type is an 8-bit signed two's complement integer. It has a minimum value of -128 and a maximum value of 127 (inclusive)

so still not uint_8 and need to convert ..

ps : in the code I wrote for proxying mwi binary to mavlink i had the same issue

Code: Select all

src/serial/serialport.c:83:2: attention : pointer targets in passing argument 1 of ‘strcpy’ differ in signedness
/usr/include/bits/string3.h:103:1: note: expected ‘char * restrict’ but argument is of type ‘uint8_t *’

[Tommie]

so lets write to an bytes[] and then call write(byte bytes[]) ?

http://www.processing.org/reference/lib ... rite_.html

byte: The byte data type is an 8-bit signed two's complement integer. It has a minimum value of -128 and a maximum value of 127 (inclusive)

so still not uint_8 and need to convert ..

No. Whether a number is negative or positive is only a matter of interpretation, we only care about the binary representation. But if course it can still be confusing at times.
So if you want to be on the safe side, use "int" and mask the upper bits:

Code: Select all

int code = 254;
byte code_byte = code & 0xFF;
System.out.println(code);
System.out.println(code_byte); //although the binary representation is the same, Java will interpret it as a negative number!
System.out.println((int)(code & 0xFF)); // now we cast it to int, forcing a different interpretation

So you can simply use a byte array, although Java will interpret the data as signed, you can safely transmit them.

Code: Select all

src/serial/serialport.c:83:2: attention : pointer targets in passing argument 1 of ‘strcpy’ differ in signedness
/usr/include/bits/string3.h:103:1: note: expected ‘char * restrict’ but argument is of type ‘uint8_t *’

Then simply use "int8_t" or use an explicit cast. If you are handling binary data, signedness is more or less meaningless.

[Tommie]

OK, I uploaded a new patch that should solve the broken MAG calibration, configuration reset etc. as well:

https://github.com/wertarbyte/multiwii- ... /fix_debug

please feel free to comment and merge.

[kos]

And these bottom 8 bits are not the same put into the string. Here is a small test case. Please try to guess the output first, then compile and run:

Code: Select all

class Chartest {
   public static void main(String[] args) {
      byte entry = (byte)0xFE; // 254 (unsigned)
      String foo = "a" + "b" + (char)entry;
      byte[] b = foo.getBytes();
      for (int i=0; i<b.length; i++) {
         int unsigned = b[i] & 0xff; // convert to unsigned number
         System.out.println(unsigned);
      }
   }
}

hum .. the byte are encoded in the current Charset

it is like guessing what is the output of :

Charset.defaultCharset().name() [1]

please refere to StringCoding class

Code: Select all

    public byte[] getBytes() {
   return StringCoding.encode(value, offset, count);
    }

on your system , what is the output of : ?

Code: Select all

System.getProperty("file.encoding")

i think this is the root cause

[Tommie]

hum .. the byte are encoded in the current Charset

Yes, and this is why you cannot safely use non-ASCII characters in a string if you want to preserve the binary representation.

i think this is the root cause

The root cause is trying to transer binary data using a string. Don't do it.

[kos]

byte: The byte data type is an 8-bit signed two's complement integer. It has a minimum value of -128 and a maximum value of 127 (inclusive)

so still not uint_8 and need to convert ..

(byte)254 is wrong



here is your test case

Code: Select all

class Chartest {
  public static void main(String[] args) {
    int entry = 0xFE; // 254 (unsigned)
    String foo = "a" + "b" + (char)entry;
    System.out.println("ref1 : " + foo);


    for (int i=0; i<foo.length(); i++) {
      int unsigned = Integer.valueOf(foo.charAt(i)); // convert to unsigned number
      System.out.println(unsigned);
    }


    entry = 254; // 254 (unsigned)
    foo = "c" + "d" + (char)entry;
    System.out.println("ref2 : " + foo);


    for (int i=0; i<foo.length(); i++) {
      int unsigned = Integer.valueOf(foo.charAt(i)); // convert to unsigned number
      System.out.println(unsigned);
    }


    byte bentry = Byte.valueOf("100"); // 100 (unsigned)
    foo = "e" + "f" + (char)bentry;
    System.out.println("ref3 : " + foo);

    for (int i=0; i<foo.length(); i++) {
      int unsigned = Integer.valueOf(foo.charAt(i)); // convert to unsigned number
      System.out.println(unsigned);
    }


    try{
      bentry = Byte.valueOf("254"); // 254 (unsigned)
      foo = "g" + "h" + (char)bentry;
      System.out.println("ref4 : " + foo);


      for (int i=0; i<foo.length(); i++) {
        int unsigned = Integer.valueOf(foo.charAt(i)); // convert to unsigned number
        System.out.println(unsigned);
      }
    }catch (Exception e) {
      e.printStackTrace();
    }

    int offset = 128;
    byte b2entry = (byte)(0xFE-offset);
    foo = "e" + "f" + (char)(b2entry);
    System.out.println("ref5 : " + foo);

    for (int i=0; i<foo.length(); i++) {
      int unsigned = Integer.valueOf(foo.charAt(i)); // convert to unsigned number
      if (i==2){
        unsigned+=offset;
      }
      System.out.println(unsigned);
    }


  }
}

[kos]

http://www.coderanch.com/t/526487/java/ ... Hex-String