simulator: Improve simulator controls

[elfringham]

Ensure that the cruise buttons and the blinkers are seen by the thread that sends the CAN messages before being overwritten. This makes them usable.

Description

After launching the simulator it was impossible to enable cruise control, the button presses were ignored.

Verification

After launching the simulator it is now possible to enable cruise control as expected.

[github-actions[bot]]

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

• Convert your PR to a draft unless it's ready to review
• Read the contributing docs
• Before marking as "ready for review", ensure:
  • the goal is clearly stated in the description
  • all the tests are passing
  • the change is something we merge
  • include a route or your device' dongle ID if relevant

[elfringham]

Hi Michel, Thanks for your questions.

Let me start by explaining how I understand what I was seeing when I run the simulator. Firstly the behaviour I was seeing was that on most occasions when I start the simulator then I could not engage the cruise control but maybe 1 time in 5 runs then the cruise control would come on by itself after a short period. But even when the cruise control came on, then I could not control the speed with the cruise buttons. This behaviour of only engaging sometimes seems to be non-deterministic and therefore suggests that there is probably a race condition causing it or else maybe an uninitialised variable.

Looking at the code I could not see any uninitialised variables. Running some tests and instrumenting the code I could see that the button presses were being received in the main thread but a corresponding CAN message is almost never sent. In fact the only relevant CAN message happened spuriously and was the cause of the cruise control engaging occasionally.

So you have the main thread receiving button presses and then clearing them again on the next pass. Also you have the update thread that will send a CAN message if it sees a change from the previous state. But there is no signalling between the two threads. There is nothing to ensure that a button press will have been seen by the update thread before the main thread clears it. And this seems to be what is happening, the two threads seem to be scheduled such that the update thread does not see the button press. On rare occasions the update thread will run after "self.simulator_state.cruise_button = 0" and before "self.simulator_state.cruise_button = CruiseButtons.DECEL_SET", and it will see that as a change from the previous state and so send a BUTTON_ENABLE message causing cruise to engage.

In order to ensure that a button press resulted in a CAN message being sent there needs to some sort of signalling between the two threads and this is my answer to your first question. The main thread sets the button state but does not clear it, the update thread will send the CAN message and clear the button state afterwards. This ensures that each button press will result in one* CAN message being sent. So that is why the function that sends the CAN messages 'edits' the simulator state, it is signalling to the main thread that the button press event has resulted in a CAN message being sent which is what is needed.

As for the blinkers, I believe that apart from ensuring that the button press results in a CAN message being sent, I have not changed the behaviour from what was intended to be achieved by the original code. My testing shows that pressing a blinker button results in a message on screen about steering to initiate a lane change when safe. Executing the steering input then results in the lane change. The main thread sets the state and the update thread clears the state after sending a CAN message. This is the same as for the cruise control buttons.

• This is not entirely thread safe, there is a small window between reading the state in the update thread and clearing it, during which time the main thread may have set a button press and that would result in that button press being missed. But it is much safer than it was. This change was a minimal edit to achieve the desired functionality. For full thread safety it would involve a much more invasive change to introduce a mutex or some other means to ensure that a button press could not be missed.

[mimi89999]

I proposed another solution in https://github.com/commaai/openpilot/pull/32150

[mimi89999]

Have you considered using a multiprocessing.Queue ? Those should be thread safe. :thinking:

[mimi89999]

Actually, why not just use a threading.Semaphore (acquire it in SimulatorBridge._run and release it in send_can_messages)?

[elfringham]

How about this? It is thread safe and no longer edits the simulator state in the CAN message function.

[elfringham]

@mimi89999 ping?

[mimi89999]

@mimi89999 ping?

I'm not affiliated with Comma AI and can't accept your PR. I was just giving my opinions.

[github-actions[bot]]

This PR has had no activity for 14 days. It will be automatically closed in 3 days if there is no activity.

[elfringham]

keepalive

[github-actions[bot]]

This PR has had no activity for 14 days. It will be automatically closed in 3 days if there is no activity.

[github-actions[bot]]

This PR has been automatically closed due to inactivity. Feel free to re-open once activity resumes.