Possible CAT driver bug

[AsbjornPettersen]

I'm looking into cat_driver.c and CatDriver_HandleProtocol()

This looks like a bug: The "CatDriver_InterfaceBufferPutData(resp,bc);" should be placed inside the "while (CatDriver_InterfaceBufferGetData(ft817.req,5))" loop !?

If there are more than one FT-817 commands only the last will send response back!

---

    } // while do loop
    CatDriver_InterfaceBufferPutData(resp,bc);
    /* Return data back */
} // if else 

}

Should be:

       CatDriver_InterfaceBufferPutData(resp,bc);
      /* Return data back */
    } // while do loop
} // if else

}

[db4ple]

Hi, I am not so sure, why I did this like it is. It might have been intentionally (or not). The FT-817 protocol is a request-response protocol. I.e. the PC talking to the TRX has to wait for the response before it sends the next request. It could have been my intention to "eat" all old requests before responding to the last one. Someone should analyze what strategy is run by hamlib in the FT817 mode.

[AsbjornPettersen]

The cat_driver_sync_data() seems to "eat" old data before the main FT-817 loop. Don't know the FT-817 command sequences, but if there many command i assume that the sender will wait and get a timeout before sending next command. Haven't studied the hamlib library much.

[AsbjornPettersen]

hamlib findings:

1. All yaesu FT-817 functions require a response on command.
2. Some functions send 2 commands, and those will fail in hamlib (Not used in mcHF).
3. The hamlib ft817_read_eeprom() expect 2 byte response, not 4 bytes in CatDriver_HandleProtocol()
4. Unsupported in CatDriver_HandleProtocol() should send NACK by implementing "default:" 5 The command FT817_A7 not found in hamlib?

Other findings:

1. The CatDriver_BlockRecv() sets the frequency "just for the show". I'm planning to create a unit test program reading setup,,, and i don't like to set the frequency without using the FT-817 "set frequence" function.

[db4ple]

2. ?
3. Problem here is that we probablly should respond with something "illegal" since we have no EEPROM simulation (i.e. always returning 2 bytes of 0 is not a good idea). Maybe we should return just one byte or nothing.
4. How to implement NACK? AFAIK no responding is the only way to "not respond" to a command.
5. A7: http://www.ka7oei.com/ft817_meow.html , required by Ham Radio Deluxe if I remember correctly.

Other findings: The cat driver does not set the frequency just for the show. It requests a new frequency to be set.

The UI_MainHandler will see the request and execute it. The MainHandler is the only place where the frequency is changed. The delay is a few ms to several dozens of ms. You can check by comparing df.tune_new with df.tune_old (if same value, frequency has been tuned).

This prevents running too many frequency changes via CAT interface. Especially the display updates would eat much time.

[AsbjornPettersen]

hamlib/yeasu/ft817.c:

2. example of functions in hamlib with 2 FT-817 commands, but not implemented in mcHF Well. If this was implemented in mcHF the hamlib code will work aftherall because if zero timeout in cat_driver! So forget this.

Code: int ft817_set_dcs_sql (RIG *rig, vfo_t vfo, tone_t code) { .... if ((n = ft817_send_icmd(rig, FT817_NATIVE_CAT_SET_DCS_CODE, data)) < 0) // cmd = 0x0c return n;

return ft817_send_cmd(rig, FT817_NATIVE_CAT_SET_DCS_ON); // cmd = 0x0a

}

4. NACK hamlib implementation:

static int ft817_read_ack(RIG *rig) {

if (FT817_POST_WRITE_DELAY == 0) // the default flaf is 0 !

char dummy;
int n;

if ((n = read_block(&rig->state.rigport, &dummy, 1)) < 0) {
    rig_debug(RIG_DEBUG_ERR, "ft817: error reading ack\n");
    return n;
}

rig_debug(RIG_DEBUG_TRACE,"ft817: ack received (%d)\n", dummy);

if (dummy != 0)
    return -RIG_ERJCTED;

endif

return RIG_OK;

}

Here hamlib read 1 byte, and 0x00 is ACK, and all other values are NACK. I don't know if the hamlib code are correct when require 1 byte response.

Found a very very harmless bug: if the function CatDriver_GetInterfaceState() return CAT_CONNECTED, then the ft817.state will be in CAT_INIT forever until disconnected or Clone debug functions are called. fix:

if (ft817.state == CAT_INIT) ft817.state = CAT_CAT; cat_driver_sync_data();

The "case 7:" / set mode / should be: case FT817_MODE_SET: / set mode /

[db4ple]

Hi,

NACK: the code responds with RIG_OK to indicate ACK (i.e. if it receives a single byte 0). In all other cases it assumes an error (timeout being one of the cases). So our current code does "NACK" as well. However, if we would send a non-zero byte for all commands we do not implement, we would return a "valid" value for all command we don't implement but which expect 1 byte return. The nature of the FT817 protocol with variable length responses and no proper common format for the responses with some kind of header/checksum etc. is kind of difficult to do right partially as we do it. So I would think not responding is the best NACK strategy so far (until we know all used commands and the proper response and implement that knowledge). Worked so far with all programs.

CAT_INIT vs. CAT_CAT: Agreed. Right now CAT_CAT and CAT_INIT are basically representing same state with 2 different names. This is not a bug, but of course not very good code. However, your fix is not good (enough). If we fix this, we should make sure that CAT_CAT represents the active handling of the FT817 CAT protocol and CAT_INIT the phase before that protocol becomes active. Requires multiple changes in some functions in CAT driver.

[AsbjornPettersen]

If the other programs works i agree that there no reason to change the protocol,

I did some unit-tests on all the commands implemented in the catdriver, and only 2 commands did not responded. FT817_NOOP (it's Ok) and FT817_MODE_SET (?).

[db4ple]

FT817_MODE_SET was not documented in the KA7OEI pages to return anything, so I did not and it works. However, I agree to return an ACK aka single "0" here is better.

[AsbjornPettersen]

Nice code changes! I like new CatDriver_HandleCommands().

But one last case with PTT. What if the user have disabled TX ?

case FT817_PTT_ON:
resp[0] = cat_driver.cat_ptt_active?0xF0:0x00; / 0xF0 if PTT was already on /

if(RadioManagement_IsTxDisabled() == false) { ts.ptt_req = true; cat_driver.cat_ptt_active = true; } bc = 1; break;

---

Should'nt it be :

case FT817_PTT_ON:
resp[0] = cat_driver.cat_ptt_active?0xF0:0x00; / 0xF0 if PTT was already on /

if(RadioManagement_IsTxDisabled() == false) { ts.ptt_req = true; cat_driver.cat_ptt_active = true; } else / TX disabled / { cat_driver.cat_ptt_active = false; resp[0] = 0xF0;
} bc = 1; break;

[db4ple]

Regarding the last comment: I don't think this is right. 0xF0 indicates that PTT is already active. So if TX is disabled we cannot send this, as it is not supposed to be active in this case. In fact, best would be to test what the real thing does if we send it the TX request if outside bands (where TX is disabled).

BTW, I had to change my mind regarding the request/response. Just yesterday in the Yahoo NG someone found out that N1MM logger+ is not working. Guess what, they send 2 commands in one request. Newest daily contains the respective changes.

[AsbjornPettersen]

Ok, You're the expert :)

Another question:

case FT817_EEPROM_WRITE: .. CatDriver_Ft817_EEPROM_Write(ee_addr+1,&ft817.req[2]);

Don't know the command, but should the req index be 3 -> &ft817.req[3] ?

[AsbjornPettersen]

Another thing: Checked in debug version !

define DEBUG_FT817

[db4ple]

Yes, should be 3. Doesn't do harm yet, but would if we have writes enabled for consecutive locations. FT817_DEBUG, yes, probably no one needs this to be enabled. Thanks for spotting.

[AsbjornPettersen]

Another thing:

The new union and funcPtr in ft817_eeprom_emul_t seems to be overcomplicated !? The funcPtr are always the same.

Smaller table, faster code:

typedef struct { ft817_ee_entry_t type; uint16_t start; uint16_t end; uint8_t value; // for FT817EE_DATA } ft817_eeprom_emul_t;

case FT817EE_FUNC: retval = CatDriver_Ft817_EEPROM_RW_Func (false, addr,data_p);

case FT817EE_FUNC: retval = CatDriver_Ft817_EEPROM_RW_Func(true, addr,data_p);

[db4ple]

Hi, yes, I know, for now it is the same. I am not sure if we keep it this way but for now it is max flexibility.

Idea was the ability to use extremely short functions for a single value (so no need for switch case). I know, I did not follow that path yet... We'll see how this works out once we have a more complete emulation. But this requires more analysis and some tools to test it with...

Refactoring this is not going to be a big problem, if we decide for it. Speed is not a real issue here. We may loose some 10 cycles per CAT driver EEPROM call (frequency I have seen is about 10 requests per second so we may be loosing 1.000 cycles per second out of 168.000.000 or 1/168.000. I don't think is going to change the game. One the other hand, yes, one should always think about the performance impact of each line of code as part of the coding process. I completely agree to the complexity reduction when it comes to reading the code, less complex is much better and in most cases the chocie for less complex code is better than to write code in way to achieve highest possible performance.

Anyway, for now I am going to keep this at it is.

I will fix the issues above and then we should close this issue and reopen a new one if more problems come along. Too long discussions about related on an issue limit the usefulness of them.