On 17/01/16 16:55, Dan Williams wrote: >> When valid responses were returned to the caller of the serial >> > command, the >> > caller itself was responsible for removing from the GByteArray the >> > data that >> > it was successfully processed (all the data in AT, just 1 message in >> > QCDM). But, >> > the same logic was missing for the case of errors; we were not >> > explicitly >> > removing the response and therefore in some cases we would see it >> > propagated >> > into the next command response. It was common to see this issue when >> > the echo >> > removal was disabled in the serial port, as in Option/HSO modems: > We don't need 'error' for the MMPortSerial parse_response class > function right? Nothing seems to use it. That might make this patch > clearer. > > So really the only place that port_serial_got_response() would get an > error would be from internal reasons (timeout, cancellation, cached > responses, and failure to send command), never from errors from the > modem itself. And in those cases, we blow away the buffer, right? >
Not really; the error given as input in port_serial_got_response() may also come from the response parsing in the AT implementation itself. E.g.: * port-serial.c: common_input_available() receives "+CME ERROR 14" * port-serial.c: common_input_available() calls the subclassed parse_response(). * port-serial-at.c: parse_response() tries to parse the text; e.g. processes unsolicited messages or converts the +CME ERROR string into a GError. * port-serial.c: common_input_available() now calls port_serial_got_response() passing down the processed response or the GError. * port-serial.c: port_serial_got_response() sets the result (GByteArray or GError) in the command context, which is what it's returned in the command_finish(). So, the "+CME ERROR 14" string is converted into a GError in port-serial-at.c: parse_response(); and that is what arrives in port-serial.c: port_serial_got_response(). Once that happens, we can remove the error text response from the GByteArray, which is what the patch does, as the GByteArray is not passed as response and therefore the caller will not clear it up. > So wouldn't that also blow away the next messages in the buffer for > something like cancellation or random internal MM errors? > If we've got unsolicited messages interleaved with the response, those should have been processed in the subclassed parse_response(), at the same time as we build the possible GError; so I don't think that should affect anything. > This part of the code is a bit hard to follow, unfortunately :( It is, yes, I hope the list of steps above clarifies a bit how it goes. -- Aleksander https://aleksander.es _______________________________________________ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
