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

Interior lights #540

Closed
wants to merge 2 commits into from
Closed

Interior lights #540

wants to merge 2 commits into from

Conversation

jdacoello
Copy link
Contributor

@jdacoello jdacoello commented Feb 14, 2023

feat: Improve the specification of interior lights to address issue #531 . Key aspects are listed below:

  • SingleConfigurableLight.vspec defined for generic lights whose color, intensity, and state can be acted on.
  • Changed previous name of AmbientLight to PerceivedAmbientLight. Previous definition was referring to the act of sensing the existing light in the ambient.
  • AmbientLight redefined as actuator.
  • Examples of configurable lights included: AmbientLight and Spotlight
  • Add new concept for interactive lights, where an effect can be selected.

Signed-off by: Daniel Alvarez-Coello
daniel.alvarez-coello@bmwgroup.com

@erikbosch
Copy link
Collaborator

Meeting notes: Please review/comment. To be discussed at meeting Feb 21st.

@erikbosch
Copy link
Collaborator

PR based on old VSS version. I suggest rebasing on latest master to make sure that shown diff is up to date.

spec/Body/Body.vspec Outdated Show resolved Hide resolved
@jdacoello jdacoello force-pushed the interior-lights branch 2 times, most recently from cbb94df to 693ff1f Compare February 21, 2023 10:43
@jdacoello
Copy link
Contributor Author

jdacoello commented Feb 21, 2023

All comments were addressed, and the proposed specification was squashed into one commit.

Note: These changes target the next major release (i.e., V4.0)

@erikbosch
Copy link
Collaborator

Meeting notes: Awaiting left/right vs driver/passenger decision

@erikbosch
Copy link
Collaborator

Meeting notes:

@jdacoello
Copy link
Contributor Author

Updated to use DriverSideand PassengerSide

@erikbosch
Copy link
Collaborator

There is a conflict that needs to be fixed

@erikbosch
Copy link
Collaborator

@jdacoello - would be great if you could update this PR to solve the conflicts

Signed-off-by: jdacoello <8550265+jdacoello@users.noreply.github.com>
@erikbosch
Copy link
Collaborator

I think the primary reason why the PR build fails is that there have been a merge from master to your topic branch, rather than a rebase on master branch, causing some merge commits to appear that does not fulfill the sign-off requirement that we require nowadays. I believe the easiest way to solve it is to start from a fresh master branch, cherry pick the wanted changes or add them to a new commit so that there is only a single commit above master and the force push the branch to github. Then there are some minor problems with missing new lines or trailing blanks in the modified files that needs to be addressed.

@jdacoello jdacoello changed the base branch from master to erikbosch/erik_power May 2, 2023 13:01
@jdacoello jdacoello changed the base branch from erikbosch/erik_power to master May 2, 2023 13:01
@erikbosch
Copy link
Collaborator

Hi @jdacoello - For convenience I created a clean PR at #587 for both of your PRs, starting from a clean master. There the build succeeds. I think we can either take that one and use it for merge. What I did was taking the latest master and then replacing file content with those from your PRs, to get two clean and signed-off commits. The only additional change was to fix to linter errors depending on old code in the files.

@erikbosch
Copy link
Collaborator

Merged by #587

@erikbosch erikbosch closed this May 3, 2023
@jdacoello jdacoello deleted the interior-lights branch May 3, 2023 14:24
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.

None yet

5 participants