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

Matlabsim #189

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Matlabsim #189

wants to merge 5 commits into from

Conversation

Tkabb
Copy link
Contributor

@Tkabb Tkabb commented Oct 18, 2017

"tomatlab.py"

Avoid disposal of initial transitions:
I proposed this change so that the transition from the 'Sinit' state to initial nodes are considered in the exportation as well. The previous code recreate transitions from successors node of 'Sinit' (already considered in the complete transition exportation) to their successors, which cause redundant transitions. In the proposed code 'Sinit' is created and transitions from it to its successors are considered as 'init_trans'.

"discrete.py"

This modifications should be done for all examples:
It seems that gr1c rises an error in Matlab code (In particular when after executing the following line ). As gr1c gives a memory position as names for states, in the mentioned line the state will have naming of an array of doubles instead of one double. So I would suggest "omega" instead as it works perfectly.

"load_discrete.m"

This definition is important before calling "load_Tulip", so it runs well and consider only the discrete case.

"load_Tulip.m"

Errors when generating Stateflow:
I was apple to generate the Matlab file and the Stateflow, but I could not run it in Stateflow because of the following error:

Chart 'TulipController' has unresolved symbols.
I think that the problem comes from the fact that the env. input signal has integer values (e.x. park in discrete example which is build using load_discrete.m to be selected randomly between 0 or 1) . Whereas for the generated TulipController, it takes as input and output, the values of True and False (not the boolean ones of matlab with first minuscule letters). Therefore, a change to the input and output values of TulipController should be done to either be 0s or 1s.
Other option: change to all minuscule letters ( change of size problem).

The lines 110..113 and 164..167 belong to the switched case, so they should be surrounded by if.

Running the simulation with no unconditional transition gives:

Chart 'TulipController' has no unconditional default path to a state.
This may lead to a state inconsistency error during runtime. You can also configure the diagnostic by clicking here.

So lines 372 to 403 are added to create one.

The indexes of the starting and ending states of transitions is set by finding the corresponding states name in the original 'TS.states'.
The reason for this is that naming of states in the original state-machine is not always sequential, whereas naming in state_handles is.

Finally, the presence of 'Sinit' and initial transitions from it are considered (after considering the modifications in 'tomatlab.py' to add them to TS structure (.m file)).

I proposed this change so that the transition from the 'Sinit' state to initial nodes are considered in the exportation as well. The previous code recreate transitions from successors node of 'Sinit' (already considered in the complete transition exportation) to their successors, which cause redundant transitions. In the proposed code 'Sinit' is created and transitions  from it to its successors are considered as 'init_trans'.
It seems that gr1c rises an error in Matlab code (In particular when after executing the following [line](https://github.com/tulip-control/tulip-control/blob/e4258ac28a093e4c8c39099daeeff8238d84a30f/contrib/matlabsim/load_tulip.m#L246) ). As gr1c gives a memory position as names for states, in the mentioned line the state will have naming of an array of doubles instead of one double. So I would suggest "omega" instead as it works perfectly.
This definition is important before calling "load_Tulip", so it runs well and consider only the discrete case.
I was apple to generate the Matlab file and the Stateflow, but I could not run it in Stateflow because of the folowing error:

1) 
Chart 'TulipController' has unresolved symbols.
I think that the problem comes from the fact that the env. input signal has integer values (e.x. park in discrete example which is build using load_discrete.m to be selected randomly between 0 or 1) . Whereas for the generated TulipController, it takes as input and output, the values of True and False (not the boolean ones of matlab with first minuscule letters). Therefore, a change to the input and output values of TulipController should be done to either be 0s or 1s.
Other option:  change to all minuscule letters ( change of size problem).

2) 
The lines 110..113 and 164..167 belong to the switched case, so they should be surrounded by if.

3) 
Running the simulation with no unconditional transition gives:

Chart 'TulipController' has no unconditional default path to a state.
This may lead to a state inconsistency error during runtime. You can also configure the diagnostic by clicking here.

So lines 372 to 403 are added to create one.

4)
The indexes of the starting and ending states of transitions is set by finding the corresponding states name in the original 'TS.states'.
The reason for this is that naming of states in the original state-machine is not always sequential, whereas naming in state_handles is.  

5) 
Finally, the presence of 'Sinit' and initial transitions from it are considered (after considering the modifications in 'tomatlab.py' to add them to TS structure (.m file)).
@johnyf johnyf self-requested a review October 19, 2017 02:29
@johnyf
Copy link
Member

johnyf commented Oct 19, 2017

Thanks for the changes. A quick comment: 8183c39 appears to be copying some changes that have been merged to branch master, and the PR changes are in the following commits (which are small and address one point each, which is good). Also, the branch Matlabsim is based off an earlier master (e.g., git diff tkabb/Matlabsim master --name-only).

If this observation is correct, then there are two ways to address it:

  1. rebase only the last four commits onto master, resolving conflicts, if any, and force push to the same branch:
# add this repo as `upstream`
git remote add upstream git@github.com:tulip-control/tulip-control
# update local `master`
git remote update
git checkout master
git merge upstream/master
git checkout Matlabsim
# rebase and force push
git rebase -i 8183c390168b40619db9564d9911e8427bc5cc48 --onto master
git push -f remote_name Matlabsim
  1. The person merging rebases.

No.2 avoids outdating review comments on GitHub, so in general works better. However, in this case the effect of commit 8183c39 will anyway be replaced by rebasing, and there are no review comments posted yet, so I recommend approach No.1.

@Tkabb
Copy link
Contributor Author

Tkabb commented Oct 24, 2017

Hi, I have installed git and followed you instructions and on git remote update I am getting:

`Fetching origin
Fetching upstream
Warning: Permanently added the RSA host key for IP address '192.30.253.113' to the list of known hosts.
git@github.com: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
error: Could not fetch upstream
`
Skipping this command, and executing for example
git rebase -i 68a88ef --onto master
an interface (with comment) is shown in which I don't know what to do or write.

Is there any easier and faster way for now? I am sorry that I am not handy with github. If not lets do approach No.2

@johnyf
Copy link
Member

johnyf commented Oct 24, 2017

@Tkabb you need to either generate an SSH key in order to use git@..., or replace that address with the HTTPS address of tulip's repository to define the remote. For the second comment, this may help.

@johnyf
Copy link
Member

johnyf commented Nov 2, 2017

@Tkabb did the changes in da60046 lead to a working example within Matlab, or are they a suggested partial solution? (I am not sure how to interpret the commit message.)

@johnyf johnyf self-assigned this Nov 2, 2017
@johnyf johnyf added the bug label Nov 2, 2017
@Tkabb
Copy link
Contributor Author

Tkabb commented Nov 7, 2017

@johnyf really sorry for the late response. Yes after da60046 the simulation in Matlab works.

The first point is a special case error caused when using mega in the synthesis. The second is also special case error that is raise when not using switched system. The third is an error that is indispensable for any cases. The fourth is a good practice for not having a false stateflow model that does not correspond to the mealy machine in case of non sequential state numbering.
The first four points are suggestion to fix errors that I was not apple to run the simulation without applying them each in it is own case except the third.

The last point (number 5) is a modification if the suggestion in 68a88ef are considered.

@johnyf
Copy link
Member

johnyf commented Nov 7, 2017

@Tkabb thanks for confirming that the simulation works, and clarifying where each change is needed.

@slivingston slivingston self-assigned this Feb 1, 2019
@slivingston
Copy link
Member

@Tkabb I am catching up on pull requests. If you still have access to Matlab, would you be willing to merge main branch into your pull request branch and confirm your changes?

I recognize this is old, and if you do not have time, then I am happy to do this using Octave, which is supposed to be "drop-in compatible" with Matlab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants