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

Do not send InstallScheduled when changing connectors to unavailable #276

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

hikinggrass
Copy link
Contributor

@hikinggrass hikinggrass commented Nov 22, 2023

This was accidentally sent all the time but is only valid if there is a transaction ongoing

Fix verified against OCTT tests
TC_L_01_CS
TC_L_02_CS
TC_L_03_CS
TC_L_05_CS
TC_L_06_CS
TC_L_07_CS
TC_L_08_CS
TC_L_10_CS
TC_L_15_CS
TC_L_18_CS

This was accidentally sent and is completely wrong here

Signed-off-by: Kai-Uwe Hermann <[email protected]>

const auto transaction_active = this->any_transaction_active(std::nullopt);
if (transaction_active) {
this->firmware_status = FirmwareStatusEnum::InstallScheduled;
Copy link
Contributor

Choose a reason for hiding this comment

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

I will test your solution. But as we are already sending InstallScheduled from our 'core', will that go well?

Anyway, will test, let you know!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might be an issue, maybe we can use another method of determining if transactions are still running that doesn't involve this part of the code in libocpp

Copy link
Contributor

Choose a reason for hiding this comment

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

This is indeed failing L_15. Because InstallScheduled is already sent (from the 'core' application), it will send it again.

It is a bit strange that all firmware statusses are sent from 'core' and only one is sent from here isn't it?
But keeping a variable that stores the last sent status will probably do the trick as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, this isn't the proper place for it. I'll remove the commit from this PR and figure something out for L_15

@hikinggrass hikinggrass force-pushed the kh-fix-wrong-install-scheduled branch from 1ac13ce to 62922d6 Compare November 23, 2023 08:20
@hikinggrass hikinggrass requested a review from maaikez November 23, 2023 08:24
@hikinggrass hikinggrass merged commit 4cff8fe into main Nov 23, 2023
@hikinggrass hikinggrass deleted the kh-fix-wrong-install-scheduled branch November 23, 2023 08:29
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.

2 participants