Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connector: Fix status after reset #5889

Merged
merged 5 commits into from
Feb 15, 2023
Merged

Connector: Fix status after reset #5889

merged 5 commits into from
Feb 15, 2023

Conversation

ShuffleWire
Copy link
Contributor

@ShuffleWire ShuffleWire commented Feb 14, 2023

After a reset, the value returned by wb_connector_is_locked was not updated, because the controller
didn't received new value.
I can't emphasis enough the fact that I'm asking for help to assess this fix, and moreover ensure that this bug is not present in others node. This could be a pattern issue, and might have been reproduced elsewhere.
However, it's seems that is a specific bug of the Connector : it's the only one to make use of both mNeedToReconfigure and a custom reset() function. I do think it's because contrary to others node that are pure sensor, this one actively impact the simulation, hence the code pattern change a bit. Am I right ?

Description
Ensure that after a reset the correct status is sent to a controller.

Related Issues
This pull-request fixes issue #5890

Tasks
Add the list of tasks of this PR.

  • Ensure that it's the correct fix
  • Ensure that the other nodes don't have this issue, and fix them accordingly

@ShuffleWire ShuffleWire requested a review from a team as a code owner February 14, 2023 07:40
@ShuffleWire ShuffleWire linked an issue Feb 14, 2023 that may be closed by this pull request
@stefaniapedrazzi stefaniapedrazzi added the enhancement Implementation of a minor feature label Feb 14, 2023
@stefaniapedrazzi stefaniapedrazzi added this to the R2023b milestone Feb 14, 2023
@stefaniapedrazzi
Copy link
Member

stefaniapedrazzi commented Feb 14, 2023

After adding this functionality to the reset, the documentation should be updated:
https://github.com/cyberbotics/webots/blob/develop/docs/reference/supervisor.md#resetreload-matrix

And an entry should be added in the ChangeLog:
https://github.com/cyberbotics/webots/blob/develop/docs/reference/changelog-r2023.md

@stefaniapedrazzi stefaniapedrazzi added bug Something isn't working test suite Start the test suite and removed enhancement Implementation of a minor feature labels Feb 14, 2023
@ShuffleWire
Copy link
Contributor Author

After adding this functionality to the reset, the documentation should be updated: https://github.com/cyberbotics/webots/blob/develop/docs/reference/supervisor.md#resetreload-matrix

I'm not sure if it's really necessary :/ what the fix does is just to revert to the correct behavior of getting things fully reset. Moreover I'm not actually changing any behavior of this node listed in this matrix, as I'm changing only the way it interact with the controller. For the actual simulation it should not make any difference.

And an entry should be added in the ChangeLog: https://github.com/cyberbotics/webots/blob/develop/docs/reference/changelog-r2023.md

done !

@stefaniapedrazzi
Copy link
Member

Yes, this is a bug fix and nothing should be added in the table.
So then it should target the master branch instead of the develop branch given that this fix doesn't break the compatibility.

After a reset, the value returned by wb_connector_is_locked
was not updated, because the controller
didn't received new value.
@ShuffleWire ShuffleWire changed the base branch from develop to master February 14, 2023 10:03
@ShuffleWire
Copy link
Contributor Author

ShuffleWire commented Feb 14, 2023

it should target the master branch instead of the develop branch given that this fix doesn't break the compatibility.

Cleanly force-pushed to rebase on master

@ShuffleWire
Copy link
Contributor Author

ShuffleWire commented Feb 14, 2023

Just as a side note, could it help with some issues I've seen about not reproducible run ? Maybe (imo) it could be worth to check if those case were impacted by this bug (ie they use a Connector and act differently based on the locked state returned by the controller)

additionally, #5332 could be affected by the type of bug, ie a handler not called (correctly) on reset, but I really don't know much, if it ring a bell for someone, would be happy to test bugfixes ! :)

@stefaniapedrazzi
Copy link
Member

The fix is fine.
We should wait before the source tests are fixed before merging.

The reset functionality is limited with respect to the reload one and it doesn't resets all the devices status.
The list of what is reset can be found here.
So rather than a bug, what is described in #5332 is more a missing functionality of the reset action.

@ShuffleWire
Copy link
Contributor Author

To be fair, I was expecting that the reload would bring the simulation to a state from which the simulation would be reproducible. Basically, if the user have changed nothing in the world, I would expect that the reset would be equivalent to the reload, in term of reproducibility of (the starting point of) the simulation.

To be clear, my use case is that I've a robot and an external controller that I'm working on, so I'm often restarting the simulation. For the most part I don't change the simulation (otherwise, yes, simulation could obviously not be reproducible), but I'm just restarting/running/stopping the controller/restarting etc over and over... And I would expect that the initial state of the simulation would be the same each run.

The reload functionality is fine, but be able to reset make it so much quicker, especially with world with many heavy object (in term of file size).

But whatever ! The fix was necessary, thank for the documentation update, it's just that simulation reproducibility is very important for my work, and webots got mostly it right. It would be sad to let it go on some edges case :)

@omichel
Copy link
Member

omichel commented Feb 15, 2023

I agree with you @ShuffleWire that we should try to make the reset functionality as close as possible to the reload functionality. Although, this is the ultimate goal, it is not easy to achieve. But your contribution is helping going towards this objective. Nevertheless, meanwhile we have to document what are the differences between reset and reload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working test suite Start the test suite
Development

Successfully merging this pull request may close these issues.

Lock connector status incorrect after reset
3 participants