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

Added Approach script #17

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Added Approach script #17

wants to merge 14 commits into from

Conversation

abiosbug
Copy link

@abiosbug abiosbug commented Sep 8, 2021

First attempt to add a PID controlled approach script

@dbaumgarten
Copy link
Contributor

I have taken a look at the commit where the tests complained about too many lines.
There was indeed a trailing newline in the file (which some editors dont't seem to display).
Opening the file in vscode clearly shows a 21th line.

Also running cat -A Scripts/Approach/approach.yolol gives:

sp=19 Kp=0.3 Ki=0 Kd=0.5 le=0 int=0 d=:Distance$
IF :Approach THEN :Cruise=0 :Mi=1 GOTO3 ELSE GOTO1 END$
IF :RFC AND :RFC<d THEN d=:RFC END $
e=d-sp de=e-le int=int+e o=Kp*e+(Ki*int)+(Kd*de) le=e$
IF d>999 OR e<10 THEN :Approach=0 GOTO8 END$
IF o>0 THEN :FCUForward=o ELSE :FCUForward=0 END$
IF o<0 THEN :FCUBackward=-o ELSE :FCUBackward=0 END$
IF :Approach THEN GOTO3 ELSE :FCUForward=0 :FCUBackward=0 END GOTO1$
$
$
$
$
$
$
$
$
$
$
$
// Approach script using pid loop, adjusted for the #JiltedOP$

(Note the $ at the end of the last line).

Tl;dr: everything works as it should work, but some editors dont show trailing LFs, leading to confusion

What editor are you using to write the script?

Without Rangefinders reporting actual distances as a result of FCUForward input, this feature cannot be tested.
Testing the PID loop logic seems doable though, feel free to add if possible
@abiosbug
Copy link
Author

abiosbug commented Sep 9, 2021

Hi, I just used the Github web interface ;-)

@dbaumgarten
Copy link
Contributor

Well, now the checks are complaining that your code has no tests.
The general rule is that all contributions need working tests, to ensure some level of quality for the code.

@martindevans
Copy link
Contributor

The general rule is that all contributions need working tests, to ensure some level of quality for the code.

We discussed this in Discord and concluded there's not a great way to test this at the moment - you'd need some kind of test harness to reduce distance as "time" passed. What's the correct way to submit a script with no tests?

@dbaumgarten
Copy link
Contributor

I don't really understand what the issue here is.

At least some basic cases should be able to be tested just fine.
Set Approach, Distance and RFC to some values and test if FCUForward gets set to reasonable values.

There is no need to check every single edge-case. Just add some basic ones to make sure nothing is completely broken.

@abiosbug
Copy link
Author

abiosbug commented Sep 10, 2021

How would I be able to run the tests locally so I don't have to force a commit just have a test run? Reason being I don't know what the exact output of the PID loop is supposed to be, is there a way to have a test condition that uses less then or greater than? As in :output { FCUForward: GTE 10 } ? Answering my own question here: https://github.com/dbaumgarten/yodk

@dbaumgarten
Copy link
Contributor

Exactly, the testing is done using the yodk, which you can use to run tests locally.
Currently there is no feature to define conditions other then "exact match".

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

Successfully merging this pull request may close these issues.

3 participants