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

Some mostly cosmetic changes to get started #63

Merged
merged 5 commits into from
Sep 5, 2024

Conversation

taliesin
Copy link
Contributor

@taliesin taliesin commented Sep 4, 2024

All but the last commit are pretty cosmetic and hopefully fit the overall style.

The helper timeout functions (last commit) try to reduce complexity a bit and there was one situation that seems like a bug, so a TODO was inserted. Please review this situation.

taliesin added 5 commits September 5, 2024 00:24
Signed-off-by: taliesin <[email protected]>
Signed-off-by: taliesin <[email protected]>
Signed-off-by: taliesin <[email protected]>
 testing charging_allowed() always leads to the same switches,
be less verbose with always the same conditions.

Signed-off-by: taliesin <[email protected]>
NOTE: in one case the standard behaviour is not used (auth_grant_to),
  this seems wrong, currently it's marked with a TODO, but not
  changed.
Signed-off-by: taliesin <[email protected]>
@taliesin
Copy link
Contributor Author

taliesin commented Sep 4, 2024

This still says 'merging is blocked', but all commits were signed off?
Not sure what I'm doing wrong.

@dzurikmiroslav
Copy link
Owner

The main branch has set Require signed commits, I turned it off and now is merge applicable...

@dzurikmiroslav dzurikmiroslav merged commit aeacddb into dzurikmiroslav:master Sep 5, 2024
8 checks passed
@dzurikmiroslav
Copy link
Owner

Merged, thanks for the code review :-)

@taliesin
Copy link
Contributor Author

taliesin commented Sep 6, 2024

Could you please reflect on the TODO in the last commit, I've marked it with a comment.

@dzurikmiroslav
Copy link
Owner

When the wallbox requires authorization to start charging. EV can be authorized before connecting to the wallbox.
By authorizing (clicking the Authorize button or sending a Modbus command, etc.), authorization is enabled for 60 seconds for the first connected EV. If the EV is disconnected within 30 seconds and another EV is connected immediately, it must be authorized again.

Therefore, auth_grant_to is always set to 0, not just when it is expired.

I know that 60 seconds is not much for replacing the EV, but I was thinking about the fact that it could also be a configurable parameter.

@taliesin
Copy link
Contributor Author

taliesin commented Sep 6, 2024

so I would say:

authorized = is_expired(&auth_grant_to);
// in any case we need a fresh authorization, if the EV is disconnected.
auth_grant_to = 0;

... would be a correct solution and be more descriptive.

@dzurikmiroslav
Copy link
Owner

dzurikmiroslav commented Sep 7, 2024

Yes, that should works correctly

@dzurikmiroslav
Copy link
Owner

So, when I wrote some unit tests, I found out the check authorized = is_expired(&auth_grant_to); not work correctly. First time when EV was not authorized it always return true. The problem was non zero value check in is_expired.
After reverting to authorized = auth_grant_to >= xTaskGetTickCount(); authorizing work correctly.

@taliesin
Copy link
Contributor Author

I'm currently too busy to work on it, but the revert somehow defies the change to is_expired(). My impression is that auth_grant_to should be initialized, so that is_expired() works.

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