Skip to content

Conversation

@dihm
Copy link
Collaborator

@dihm dihm commented Apr 12, 2024

This PR has adm respond with ready\r\n before listening for the binary read. This allows the user to check for issues in the command, without incurring the timeout penalty during normal operation.

It also prevents bugs where erroneous commands did not stop execution of the adm command.

Finally, I updated the examples in the readme to be python scripts, hopefully making the add and adm commands a bit easier to use.

dihm added 4 commits April 11, 2024 20:30
This removes the need to send a separate `cls` before doing `adm` commands.
…ry writes.

This forces the user to check for potential errors in the command without
incurring a readlines timeout penalty if there are no errors.

Also adds example to the readme of how a binary write works.
@dihm dihm added bug Something isn't working documentation Improvements or additions to documentation labels Apr 12, 2024
@dihm dihm requested a review from carterturn April 12, 2024 13:34
@carterturn
Copy link
Collaborator

While we are handling the errors, I think it would also be good to take care of the invalid reps number case.
At the moment, I think this is handled in a very bad way: if the reps number is greater than zero and less than 5, it will subtract four anyway, which will loop around to ~2^32 reps.
As we do commit to reading all the expected commands, we should probably not stop the adm when this happens, and instead log the error and report it once it is over. I think I see three ways of handling this:

  1. skip the insert, increment do_cmd_count, warn
  2. skip the insert, don't increment do_cmd_count, warn
  3. set reps to 5, warn
    At the moment, I think 1 is the best solution.
    It would probably be more user friendly to have the warning include the index of the problematic command(s), but this also increases the error handling complexity by quite a bit. As a compromise, we could just include the index of the most recent error and the number of problems.

Besides this, I think this looks good.

@carterturn
Copy link
Collaborator

Actually, having attempted to implement this, I think the third option above (with 0 reps or 5 reps) is actually the least problematic output if someone does not check for error messages.

@dihm
Copy link
Collaborator Author

dihm commented Apr 23, 2024

Actually, having attempted to implement this, I think the third option above (with 0 reps or 5 reps) is actually the least problematic output if someone does not check for error messages.

@carterturn Thanks for tackling that. I had considered doing something about those error prints, but avoided it as I didn't have a very good idea. What you've implemented is simple and effective. Ultimately, there really shouldn't be reps errors in the binary write, and if there are it will likely be easier for the user to fix their table and re-write the whole thing than try to fix individual commands.

I'll fix the typo in the readme then merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants