Fix: Planewave EFA char sign

[comptonizing]

The Planewave EFA (apparently) writes unsigned chars to the serial bus
instead of signed chars as usually assumed. With this fix the data is
interpreted in the correct way. This should also address
#1173 .

[knro]

I don't think this is required all over the place. A byte is a byte whether it is char or unsigned char. I believe we should only fix the calculateChecksum function to perform the correct cast to uint8_t

[comptonizing]

Hi Jasem,
if I observed the behavior of the focuser correctly also the reported position was jumping arbitrarily betwenn the actual meaningful position and small, negative numbers, i.e. -255 to -1. In my opinion this behavior indicates that not only the calculation and interpretation of the checksum is incorrect but also of all the other data.
Regarding your comment of a char being a char, I have to disagree. Consider the following example:

  unsigned char sent = 0xAF;                                                    
  char read = sent;                                                             
  printf("%d\t%d\n", sent, read);   

which prints

175	-81

I guess that this is also the problem in the case of the EFA driver.

[knro]

Both of these variables are stored as 0xAF in memory. Only when you use printf or use them in a math operation as is that it matters. What can be done is during the checksum, the values are casted to uint8_t first. This should ensure the calculations are correct.

[comptonizing]

I tried it already, since this was also my first idea, but it didn't work. When doing so the checksums are fine, but the value of the reported focuser position jumps between the real position and a small, negative number, i.e. -255 to -1.
I constructed the following example inspired by the actual current implementation:

  char c1 = 0;                                                                  
  char c2 = 0;                                                                  
  char c3 = 0xAF;                                                               
                                                                                
  unsigned char uc1 = c1;                                                       
  unsigned char uc2 = c2;                                                       
  unsigned char uc3 = c3;                                                       
                                                                                
  uint32_t position = 0;                                                        
  position = c1 << 16 | c2 << 8 | c3;                                           
  uint32_t uposition = 0;                                                       
  uposition = uc1 << 16 | uc2 << 8 | uc3;                                       
                                                                                
  printf("%u %u\n", position, uposition); 

In both cases I use uint32_t to store the final number by setting the individual bytes starting from the LSB (I think). printf in both cases gets the unsigned 32 bit integer and also interpretes it as an unsigned integer. Note that both positions are initialized to 0 before that. But what comes out of it is

0xffffffaf 0xaf

Personally, I would have expected the right one. Note also that this goes good up to c3=0x7F, which is the maximal value for a signed char. I tested the version of the pull request and as I said, reading the focuser position now works perfectly.
Consider also what planewave writes about the calculation of the temperature:

Conversion of the two received bytes to Celcius

int rawTemp = byte2*256 + byte3
bool tempIsNeg = false
if(rawTemp > 32768)
{
  tempIsNeg = true
  rawTemp = 65536 - rawTemp
}
int intPart = RawTemp / 16
int fractionDigit = (RawTemp - intPart) * 625 / 1000
float celciusTemp = intPart + fractionDigit / 10
if(tempIsNeg) celciusTemp = -celciusTemp

See http://planewave.com/files/tech/efa-protocol.html They write that if the raw temperature exceeds 32768 the temperature actually is negative. Wouldn't this also indicate the same problem? I think that this is also the reason for Issue C in #1173 which is also working when reading this values as unsigned chars.

[knro]

Ok thank you for taking the time to detail everything. Can you please apply this patch and try it out? It's similar to your PR.

efa.txt

[comptonizing]

Thank you very much, I just tried the patch. For some bizarre reason the minus in front of the sum in the checksum calculation has to stay. If it is removed, as you did, I get a a timeout while trying to connect. I only know this because it's also something I already tried a few days ago while trying to figure out what's going on.
With the minus still in place the behavior is the same is in my patch, so reading all parameters from the focuser is working, meaning the position and temperature. Also movement relative and absolute is working fine. However, as I already wrote in #1173 a few odd things remain:

1. When the driver tries to read the calibration state from the focuser I get a read timeout. Maybe only certain types of focusers support this?
2. When using the hand controller in conjunction with the indi driver, a lot of random invalid checkums are created. Everything continues to work fine, however
3. Issue B of Issues with Planewave EFA driver #1173 sometimes also shows up. What I mean is that I have to re-enter the maximum position in encoder steps everytime the driver is started, but I often can not enter the last or the last two digits of that rather large number.

Thank you very much for your support.

[knro]

1. I'm not sure, this is possible. You can turn tty_debug(1) and see if there is any response at all? It's expected 7 characters, so maybe it sends less?
2. Not sure, I presume the control starts sending other stuff in the serial port that is not expected by the driver. Probably the driver needs to intercept that and handle it.
3. I think I fixed it in my local copy. But let's try to figure out Make core compile #1 and Make standard 3rdparty set compile from github clone.  #2 as well.

[knro]

Going to close this now and let's see if we need another one to fix the remainder of the issues.