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

Fix name and protocol changes within simonaAPI #385

Merged
merged 11 commits into from
Oct 19, 2022

Conversation

danielfeismann
Copy link
Member

@danielfeismann danielfeismann commented Oct 17, 2022

resolves #384

Mainly based on previous work of @sebastian-peter

@danielfeismann danielfeismann added the dependencies Pull requests that update a dependency file label Oct 17, 2022
@danielfeismann danielfeismann self-assigned this Oct 17, 2022
@danielfeismann danielfeismann changed the title Fix name changes Fix name and protocol changes within simonaAPI Oct 17, 2022
@sebastian-peter sebastian-peter marked this pull request as ready for review October 18, 2022 14:47
Copy link
Contributor

@t-ober t-ober left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice ! Just a few minor things 🙂

Comment on lines 339 to +342
val (tickInterval, lastEvs) =
getTickIntervalAndLastEvs(currentTick, modelBaseStateData)
getTickIntervalAndLastEvs(tick, modelBaseStateData)

validateDepartures(lastEvs, requestedDepartingEvs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does last evs refer to the evs currently at the evcs upon request ? I think currentEvs would be clearer. If you agree we should als adapt the method signature of getTickIntervalAndLastEvs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't agree to that since the EVs retrieved here stem from a tick in the past, thus lastEvs. This is in contrast to the EVs that are determined for the current tick, namely the ones staying plus arrivals, thus currentEvs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also makes sense if you look at the naming of currentTick, which is the tick currently activated. A tick at which the agent was active before the current tick I would name lastTick :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the point (and was expecting that you will make it 😄 ) but I would understand it a bit differently.
Since we haven't dealt with arrivals and departures yet, the current evs are still the evs from last tick. Nevertheless I don't think we have to drag that on here.

@sebastian-peter sebastian-peter self-assigned this Oct 19, 2022
sebastian-peter and others added 2 commits October 19, 2022 10:29
Co-authored-by: t-ober <63147366+t-ober@users.noreply.github.com>
@t-ober t-ober merged commit 460f293 into dev Oct 19, 2022
@t-ober t-ober deleted the df/#384-fix-simonaAPI-breaking branch October 19, 2022 10:12
@sebastian-peter sebastian-peter added this to the Version 3.0 milestone Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix breaking SIMONA due to renamed messages and changed protocol in simonaAPI
3 participants