break up the gigantic loops; a few misc fixes

[merlinran]

It replaces https://github.com/Twisted-Fields/acorn-precision-farming-rover/pull/3 for better code diff.

It breaks the main loops of remote_control_process.py (nearly 1000 lines of code!) and master_process.py into more manageable methods each within 100 lines. Shorter methods are easier to reason about and have proper level of abstraction.

To make sure I didn't break things, I ran the new and old code side by side, and didn't see difference on the behaviour, except a few bugs of the old code which have been fixed in this PR. I wasn't able to make acorn really move in the simulation with this PR, due to a problem exists in the old code too(travel_speed is always 0), but after hardcoding the default nav parameter travel_speed, the simulated Acorn could rover could follow any path back and forth. Some bugs may be revealed only when running on real hardware, though I'd be surprised if there are many.

[tlalexander]

A note about code style. I prefer verbose if statements to one liners. eg:

if len(self.nav_path.points) == 2:
    start_index = 0
else:
    start_index = int(len(self.nav_path.spline.points)/2) - 5

vs

start_index = 0 if len(self.nav_path.points) == 2 else int(len(self.nav_path.spline.points) / 2) - 5

I suppose we can work up some style guidelines at some point. Or if google has a public style guide probably following theirs would make sense.

[merlinran]

yeah this point is fair enough. There are simpler cases like status = "fix" if data[5] == "1" else "no fix" where one liners might make sense for more concise code, but anything beyond that should be if statements. I'll fix the stuck out ones.

[tlalexander]

This is a nice refactor! Finally digging in to it today. You really did clean it up a lot thank you!

I've discovered the "drive_solution_okay" logic is not the same. The following condition produces no solution in your streamlined code but does work in the main branch.

Could not find drive solution. Disabling autonomy.
12/01/2021 08:50:52 PM - main.remote - ERROR - calculated_rotation: 87.06461285743364, vehicle_travel_direction Direction.FORWARD, path_following_direction Direction.EITHER

It can be convinced to move if you include the possibility of path direction Either.

        drive_solution_okay = (vehicle_dir == Direction.EITHER or
                               vehicle_dir == path_dir or
                               path_dir == Direction.EITHER and
                               abs(calculated_rotation) <= _MAXIMUM_ROTATION_ERROR_DEGREES)

However this still does not produce the same behavior - it looks like it's turning the wrong direction. It can be manually debugged, but I wonder, do you have experience writing tests? I would like to introduce unit tests but I'm not great with them and am not sure where to start in setting up the framework.

If you were open do it, writing a test for the old function and then making sure the new function has the same behavior would be great. I like the cleanliness of your refactor but the somewhat tricky logic of the code must be preserved.

Or if writing tests is too much for now, we could revert this function to the old form.

Thanks!

[tlalexander]

Did a manual debug and fixed it! I still need to test this code on the real vehicle. The code does throw an error when you run master_process.py without the --sim flag so something is up. Relevant bits:

  File "/home/pi/vehicle/remote_control_process.py", line 1243, in run_control
    remote_control = RemoteControl(remote_to_main_lock, main_to_remote_lock,
  File "/home/pi/vehicle/remote_control_process.py", line 220, in __init__
    self.gps = gps.GPS(self.logger, self.simulated_hardware)
  File "/home/pi/vehicle/gps.py", line 20, in __init__
    self.rtk_process.launch_rtk_sub_procs(self.logger)
AttributeError: 'GPS' object has no attribute 'rtk_process'

[merlinran]

Oops sorry! I should have not changed any logic at all in this PR, just formatting and extracting method. And yeah, adding test coverage will be my focus next given that I understand the code better now. I thought that breaking into smaller methods would allow me to write unit tests, but I realized that the better way is to write integration tests first before any significant changes.

The fix to AttributeError is simple: The code should be rtk_process.launch_rtk_sub_procs(self.logger) rather than self.rtk_process.launch_rtk_sub_procs(self.logger). Another careless mistake 😳

Ping me if anything comes up when testing on the vehicle!

[merlinran]

I actually triple checked the drive_solution_okay code, but apparently still got error, so embarrassing. I can't resist the temptation to do it again, as careful as I can.

The old code can be rewritten as the following pseudocode:

drive_solution_okay = True
if vehicle_dir == FORWARD and ((
    abs(calculated_rotation) > _MAXIMUM_ROTATION_ERROR_DEGREES
    and path_dir == FORWARD
    and abs(calculated_rotation) > _MAXIMUM_ROTATION_ERROR_DEGREES
) or path_dir == BACKWARD):
    drive_solution_okay = False

if vehicle_dir == BACKWARD and ((
    abs(calculated_rotation) > _MAXIMUM_ROTATION_ERROR_DEGREES
    and path_dir == BACKWARD
    and abs(calculated_rotation) > _MAXIMUM_ROTATION_ERROR_DEGREES
) or path_dir == FORWARD):
    drive_solution_okay = False

Consider that vehicle_dir and path_dir have only 3 states, the two ifs can be merged as:

drive_solution_okay = True
if vehicle_dir != EITHER and ((
    abs(calculated_rotation) > _MAXIMUM_ROTATION_ERROR_DEGREES
    and path_dir == vehicle_dir
    and abs(calculated_rotation) > _MAXIMUM_ROTATION_ERROR_DEGREES
) or (path_dir != EITHER and path_dir != vehicle_dir)):
    drive_solution_okay = False

Removing the redundant items and reordering a bit, it can be further simplified as:

drive_solution_okay = True
if vehicle_dir != EITHER and (
    (path_dir == vehicle_dir and abs(calculated_rotation) > _MAXIMUM_ROTATION_ERROR_DEGREES) or
    (path_dir != vehicle_dir and path_dir != EITHER)):
    drive_solution_okay = False

Finally, we can make it a single assignment:

drive_solution_okay = not (
    vehicle_dir != EITHER and (
        (path_dir == vehicle_dir and abs(calculated_rotation) > _MAXIMUM_ROTATION_ERROR_DEGREES) or
        (path_dir != EITHER and path_dir != vehicle_dir)))

Now comes boolean algebra, one step at a time:

drive_solution_okay = (not (vehicle_dir != EITHER) or not (
        (path_dir == vehicle_dir and abs(calculated_rotation) > _MAXIMUM_ROTATION_ERROR_DEGREES) or
        (path_dir != EITHER and path_dir != vehicle_dir)))

drive_solution_okay = (vehicle_dir == EITHER or (
        not (path_dir == vehicle_dir and abs(calculated_rotation) > _MAXIMUM_ROTATION_ERROR_DEGREES) and
        not (path_dir != EITHER and path_dir != vehicle_dir)))

drive_solution_okay = (vehicle_dir == EITHER or (
        (not (path_dir == vehicle_dir) or not (abs(calculated_rotation) > _MAXIMUM_ROTATION_ERROR_DEGREES)) and
        (not (path_dir != EITHER) or not (path_dir != vehicle_dir))))

drive_solution_okay = (vehicle_dir == EITHER or (
        (path_dir != vehicle_dir or abs(calculated_rotation) <= _MAXIMUM_ROTATION_ERROR_DEGREES) and
        (path_dir == EITHER or path_dir == vehicle_dir)))

drive_solution_okay = (vehicle_dir == EITHER or (
        (path_dir == vehicle_dir or path_dir == EITHER) and
        (not (path_dir == vehicle_dir) or abs(calculated_rotation) <= _MAXIMUM_ROTATION_ERROR_DEGREES)))

Which, based on Wolfram Alpha, can be also written as:

# A or ((B or C) and (!B or D))  => A or ((B and D) or (!B and C)

drive_solution_okay = (vehicle_dir == EITHER or 
    ((path_dir == vehicle_dir) and abs(calculated_rotation) <= _MAXIMUM_ROTATION_ERROR_DEGREES) or
    (not (path_dir == vehicle_dir) and path_dir == EITHER))

drive_solution_okay = (vehicle_dir == EITHER or
    ((path_dir == vehicle_dir) and abs(calculated_rotation) <= _MAXIMUM_ROTATION_ERROR_DEGREES) or
    (path_dir != vehicle_dir and path_dir == EITHER))

Since path_dir == EITHER already implies path_dir != vehicle_dir, finally, we have:

drive_solution_okay = (vehicle_dir == EITHER or path_dir == EITHER or
    path_dir == vehicle_dir and abs(calculated_rotation) <= _MAXIMUM_ROTATION_ERROR_DEGREES)

Created a quick PR to fix this as well as AttributeError https://github.com/Twisted-Fields/acorn-precision-farming-rover/pull/11, but again, I shouldn't have done the "simplification" without test coverage.

[merlinran]

Again, I could enable autonomy in simulation, but only after hardcoding travel_speed at https://github.com/Twisted-Fields/acorn-precision-farming-rover/blob/main/vehicle/remote_control_process.py#L221 . I guess there should be some correlation between travel_speed and autonomy_velocity, but I'll stop messing up the code without fully understand and good test coverage 😄

[tlalexander]

Thank you so much! I will review now but this looks great. I should have clarified: not long ago I switched from hard coding the velocity to including the velocity in the path object stored in the database. It can actually store a path as a list of segments with different velocities for each one. I updated my local database but failed to commit it to github. I added the correct database to the git repo this week, but I never told you how to use it! I just pushed a commit to main with this path preloaded, but the path named "aaa_test" is the one that has the velocities pre encoded.

Also there are two types of paths in the database. Legacy paths are just a list of gps coordinates, and they should be loaded with a default velocity. Newer paths are a path object and when one of those is loaded it should use the velocity encoded in the path. During development some paths may have been stored with zero velocity, but perhaps my newest database has those overwritten.

Finally my velocity changes mean that the velocity setting on the web page may not work as expected or at all. That needs to be redesigned either with absolute limits or a scaling factor or both to support path lists with different velocities.

I will merge this PR shortly.

[merlinran]

Thanks for the details! All make sense. I also noticed the legacy vs newer path thing, but wasn't aware that velocity is encoded in the path. In the long run, I wonder if the velocity should be determined automatically from some traits of the path segments, like the curvature or the slope, or the actual task, but well, nothing to worry about right now.

[tlalexander]

Well the system is built such that another program could make those determinations when saving the path into the database. But being able to set different attributes for different segments is useful. For example once we have tools, we may need to raise the tool for movement on roads, where Acorn could drive faster, and then once it gets to the field it could lower a tool and move slower. Long term parts of that can be automatic but in the simplest sense being able to encode that per path segment should be pretty useful. We can actually encode the path following control system parameters per segment too!