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.

Sign up for GitHub

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

Jump to bottom

cross_rs4s configuration changes #497

Closed
wants to merge 2 commits into from
Closed

cross_rs4s configuration changes #497

wants to merge 2 commits into from

Conversation

pksoft72
Copy link

No description provided.

SwarcoPalm
SwarcoPalm reviewed Jan 16, 2025
View reviewed changes
Copy link

@SwarcoPalm SwarcoPalm left a comment

Choose a reason for hiding this comment

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

How can it be verified here that the controller actually goes through its start-up sequence if other states are accepted?

@emiltin emiltin added the testhub Set this label to trigger test on test hub equipment label Jan 16, 2025
emiltin
emiltin reviewed Jan 16, 2025
View reviewed changes
.github/workflows/cross_rs4s.yaml Outdated
pull_request:
types: [ opened, reopened, synchronize, labeled ]
schedule:
# schedule runs only on the default branch. time is in UTC.
# * is a special character in YAML so you have to quote this string.
# run every night at 10:00PM UTC.
- cron: '0 22 * * *'
push:
branches:
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to run tests on equipment on merges to branches. Instead we run when the tag 'testhub' is set on a PR, and on schedule.

@emiltin
Copy link
Contributor

emiltin commented Jan 16, 2025

Test are now running (since I added the 'testhub' tag to the PR), but for some reason the controller does not connect, or a process is blocking the address/port:

Aborting: Site did not connect within 30s


Finished in 30.06 seconds (files took 0.87623 seconds to load)
0 examples, 0 failures, 1 error occurred outside of examples

1 sentinel warnings:

  1 Errno::EADDRINUSE

@emiltin
Copy link
Contributor

emiltin commented Jan 16, 2025

How can it be verified here that the controller actually goes through its start-up sequence if other states are accepted?

Is this related to a change in this PR or a more general question? I don't see changes in this PR related to the startup sequence.

@pksoft72
Copy link
Author

Aborting: Site did not connect within 30s ---> I was running another test on the same controller.

Startup sequence

emiltin
emiltin reviewed Jan 17, 2025
View reviewed changes
.github/workflows/cross_rs4s.yaml Outdated
@@ -19,7 +19,7 @@ jobs:
runs-on: [ self-hosted, Linux, X64, cross-rs4s ]
strategy:
matrix:
core: ['3.1.2', '3.1.3', '3.1.4', '3.1.5', '3.2.0', '3.2.1', '3.2.2']
core: ['3.2.2', '3.1.2', '3.1.3', '3.1.4', '3.1.5', '3.2.0', '3.2.1'] # test last version first
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to start with the latest, but let's order them all as newest to oldest

emiltin
emiltin reviewed Jan 17, 2025
View reviewed changes
config/cross_rs4s.yaml
@@ -34,8 +34,8 @@ components:
CZ+76000=101DL002:
CZ+76000=101DL003:
items:
plans: [1,2,3,4,5]
traffic_situations: []
Copy link
Contributor

Choose a reason for hiding this comment

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

The test "Traffic Situation is set with M0003" is skipped because no traffic situations configured here. Can some be configured, or does the controller not support this?

Copy link
Author

Choose a reason for hiding this comment

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

Controller accepts any traffic situation, behavior is dependent on traffic solution.
I understand traffic situation as some information, which is used to select correct plan.
So it is possible in script(=user traffic logic) use current traffic situation to select different plan.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, traffic situations are something you configure on the device and can switch between, like signals plans. @otterdahl can you confirm this?

@emiltin
Copy link
Contributor

emiltin commented Jan 17, 2025

Is the controller an RS4S or RS4?

@pksoft72
Copy link
Author

Is the controller an RS4S or RS4?

RSMP works on RS4S and RS4T. RS4 is old, not in production. RS4T is upgraded RS4.
RS4S and RS4T have identical firmware so their RSMP compatibility is the same.

@emiltin
Copy link
Contributor

emiltin commented Jan 20, 2025

Could you please squash to either a single commit, or a few relevant commits?

@emiltin
Copy link
Contributor

emiltin commented Jan 20, 2025 •
edited
Loading

Core 3.2.2 and 3.2.0 succeeds, but all other versions fail, including 3.2.1 and 3.1.x. Most has these tests failing:

rspec ./spec/site/tlc/modes_spec.rb:165 # Site::Traffic Light Controller Operational yellow flash can be activated with M0001
rspec ./spec/site/tlc/modes_spec.rb:180 # Site::Traffic Light Controller Operational yellow flash affects all signal groups
rspec ./spec/site/tlc/modes_spec.rb:228 # Site::Traffic Light Controller Operational dark mode can be activated with M0001
rspec ./spec/site/tlc/modes_spec.rb:243 # Site::Traffic Light Controller Operational yellow flash be used with a timeout of one minute
rspec ./spec/site/tlc/signal_groups_spec.rb:88 # Site::Traffic Light Controller Signal Group follow startup sequence after yellow flash
rspec ./spec/site/tlc/traffic_situations_spec.rb:11 # Site::Traffic Light Controller Traffic Situation is read with S0015

Do you know what's causing some versions to fails?

@pksoft72
Copy link
Author

Could you please squash to either a single commit, or a few relevant commits?

I have changed it to single commit.

@emiltin
Copy link
Contributor

emiltin commented Jan 21, 2025

@pksoft72 Should we merge this now, and then open new issues for any remaining failing tests?

@pksoft72
Copy link
Author

@pksoft72 Should we merge this now, and then open new issues for any remaining failing tests?

You can merge now, I suppose other failing tests should be problem on controller's side.

@emiltin
Copy link
Contributor

emiltin commented Jan 21, 2025

Replaced by #504 (same content, but rebased on main).

@emiltin emiltin closed this Jan 21, 2025
@emiltin
Copy link
Contributor

emiltin commented Jan 21, 2025

@pksoft72 for your information, using githubs online merge tool committed to the main branch of your repo, which was a bit surprising to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SwarcoPalm SwarcoPalm SwarcoPalm left review comments

@emiltin emiltin emiltin left review comments

Assignees
No one assigned
Labels
testhub Set this label to trigger test on test hub equipment
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pksoft72 @emiltin @SwarcoPalm