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

polestar soc:trap failing api in fetch_soc #1614

Merged
merged 6 commits into from
Jun 12, 2024

Conversation

isomacM
Copy link
Contributor

@isomacM isomacM commented May 14, 2024

Hallo,

mir ist aufgefallen, dass das Modul einen Fehler schmeisst, wenn die Authorisierung scheitert, z.B.

2024-05-07 01:33:44,450 - {modules.vehicles.polestar.api:21} - {ERROR:fetch soc_ev0} - query_params:not yet authenticated 2024-05-07 01:33:44,461 - {modules.common.fault_state:49} - {ERROR:fetch soc_ev0} - Polestar2: FaultState FaultStateLevel.ERROR, FaultStr <class 'TypeError'> ("'NoneType' object is not subscriptable",), Traceback: Traceback (most recent call last): File "/var/www/html/openWB/packages/modules/common/configurable_vehicle.py", line 66, in update car_state = self._get_carstate_by_source(vehicle_update_data, source) File "/var/www/html/openWB/packages/modules/common/configurable_vehicle.py", line 109, in _get_carstate_by_source return self.__component_updater(vehicle_update_data) File "/var/www/html/openWB/packages/modules/vehicles/polestar/soc.py", line 20, in updater return api.fetch_soc( File "/var/www/html/openWB/packages/modules/vehicles/polestar/api.py", line 94, in fetch_soc soc = bat_data['batteryChargeLevelPercentage'] TypeError: 'NoneType' object is not subscriptable

Deshalb würde ich die Variablen soc und est_range vorbelegen.
Momentan habe ich -1 gesetzt aber vielleicht habt ihr für diesen Fall einen anderen Wert vorgesehen?

Gruß,
Philipp

Copy link
Contributor

@LKuemmel LKuemmel left a comment

Choose a reason for hiding this comment

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

Die Fehlerbehandlung erfolgt zentral in

if ev.data.get.fault_state == 2:
Sonst gibst Du hier -1 zurück, ein anderes Modul None oder 0.
Der Kontexthandler
with SingleComponentUpdateContext(self.fault_state):
fängt die Exception und setzt den fault_state auf 2 und den exception-String in fault_str. Deshalb die Exception nicht abfangen. In Deinem PR wird der SoC auf -1 gesetzt und der Benutzer fragt sich, warum kein SoC ankommt. Steht der fault_state auf 2, wird der fault_str auf der Status-Seite angezeigt.

Die Fehlerbehandlung in polestar/api.py muss angepasst werden:

  • In Z16. das req.get_http_session() verwenden. Dort wird in einem Callback geprüft, ob ein Fehler aufgetreten ist und eine Exception geworfen.
  • In Z.32 dann:
        except Exception as e:
            if result.status_code == 401:
                self.auth.delete_token()
            if self.auth.access_token is not None:
                # if we got an access code but the query failed, VIN could be wrong, so let`s check it
                self.check_vin()
            raise e
  • Z 36-40 können dann weg.

@LKuemmel
Copy link
Contributor

Ich habe das in den Wiki-Beitrag mit aufgenommen: #1616

@isomacM
Copy link
Contributor Author

isomacM commented May 15, 2024

Ich muss zugeben, dass das meine bescheidenen Programmierkenntnisse übersteigt...

Das req.get_http_session() ist jetzt eingebaut, das jetzt überflüssige check_vin() in get_battery_data habe ich rausgenommen.

packages/modules/vehicles/polestar/api.py Outdated Show resolved Hide resolved
Comment on lines 92 to 97
# preset values will be returned if api fails
soc = 0
est_range = 0
if bat_data is not None:
soc = bat_data['batteryChargeLevelPercentage']
est_range = bat_data['estimatedDistanceToEmptyKm']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# preset values will be returned if api fails
soc = 0
est_range = 0
if bat_data is not None:
soc = bat_data['batteryChargeLevelPercentage']
est_range = bat_data['estimatedDistanceToEmptyKm']
soc = bat_data['batteryChargeLevelPercentage']
est_range = bat_data['estimatedDistanceToEmptyKm']

Default-Werte werden zentral gesetzt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was heisst zentral? Das geht doch über die fetch_soc Funktion und die muss doch irgendwelche Werte übergeben. Auch hier soll eine unnötige Exception (und ein überflüssiger log Eintrag) vermieden werden

Copy link
Contributor

Choose a reason for hiding this comment

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

In welchen Fällen ist bat_data == None? Wenn die Abfrage fehlgeschlagen ist. Und dieser Fehlschlag soll geloggt werden und dem Benutzer im UI angezeigt werden, damit er weiß, dass die SoC-Abfrage fehlgeschlagen ist.
Das setzt voraus, dass die fetch_soc-Funktion die Exception nicht abfängt und die Werte nicht mit einem Standart-Wert füllt.
Der Zweig von if bat_data is not None: sollte nie erreicht werden, denn in diesem Fall muss die Funktion mit einer Exception verlassen worden sein, damit die Fehlerbehandlung das Loggen der Exception und die Meldung für das UI durchführen kann.

Copy link
Contributor

Choose a reason for hiding this comment

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

Auch hier soll eine unnötige Exception (und ein überflüssiger log Eintrag) vermieden werden

Was meinst Du mit unnötig? Wenn die Abfrage fehlschlägt, ist die Exception doch nicht unnötig, sondern nötig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ich hatte das noch nicht verstanden, dass die Funktion dann verlassen wird

@LKuemmel
Copy link
Contributor

In Z 20/21 entweder die weitere Verarbeitung abbrechen und eine Exception werfen raise Exception("query_params:not yet authenticated") oder auf die fehlende Authentifizierung reagieren, indem diese nochmal durchgeführt wird oder was im jeweiligen Fall Sinn macht.
Ohne Authentifizierung die Funktion weiter abzuarbeiten, führt voraussehbar zu keinem brauchbaren Ergebnis und sollte deshalb durch eine Exception abgefangen werden.

@LKuemmel
Copy link
Contributor

req.get_http_session() kümmert sich um das Logging der Antwort bei einem Request. Z. 45 kann damit auch entfallen.

@LKuemmel LKuemmel added this to the 2.1.5 Step 1 milestone Jun 7, 2024
@LKuemmel LKuemmel merged commit 20054ad into openWB:master Jun 12, 2024
1 check passed
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