-
Notifications
You must be signed in to change notification settings - Fork 176
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
kostal: more debug output to get better understanding of temporal behaviour #644
kostal: more debug output to get better understanding of temporal behaviour #644
Conversation
68deed9
to
def942e
Compare
#ifndef DEBUG_KOSTAL_RS485_DATA | ||
#define DEBUG_KOSTAL_RS485_DATA | ||
#endif | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we should just drop DEBUG_KOSTAL_RS485_DATA
and use DEBUG_VIA_USB
for everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we have still heavy debugging going on, i prefer keeping DEBUG_KOSTAL_RS485_DATA and DEBUG_VIA_USB separated.
But that drop dropped is good, as i just figured out, that we must replace zero bytes of RX frame before rx processing, as otherwise we don't process eg. this: if (RS485_RXFRAME[6] == 0x5E) when RXFRAME[7] is 0x00
Also rx frame length may be more than 10 bytes, as there is sent EPOCH timestamp on one frame (actually never seen that, but seen on BYD code that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that drop dropped is good, as i just figured out, that we must replace zero bytes of RX frame before rx processing, as otherwise we don't process eg. this: if (RS485_RXFRAME[6] == 0x5E) when RXFRAME[7] is 0x00
Yes, I'm handling that in my WIP branch: https://github.com/lewurm/Battery-Emulator/blob/1df27f4444ec0cfac8822256208db3e5ff780c06/Software/src/inverter/KOSTAL-RS485.cpp#L244-L249 and https://github.com/lewurm/Battery-Emulator/blob/1df27f4444ec0cfac8822256208db3e5ff780c06/Software/src/inverter/KOSTAL-RS485.cpp#L492-L497
But I don't want to submit that change just yet, because apparently some people have success with the current state and I want to understand why by adding additional logging. I suspect that it works by chance, because we send the right answer after a timeout:
Battery-Emulator/Software/src/inverter/KOSTAL-RS485.cpp
Lines 266 to 268 in 4232da8
if ((currentMillis - B1_last_millis) > INTERVAL_1_S) { | |
send_kostal(frameB1b, 8); | |
B1_delay = false; |
Also rx frame length may be more than 10 bytes, as there is sent EPOCH timestamp on one frame (actually never seen that, but seen on BYD code that)
That should be covered by the dump_rs485_data_rx(" (!RX_allow)");
case, at least it would print it in several steps then. But indeed, I haven't seen it in any traces yet.
def942e
to
0e4ec96
Compare
@dalathegreat this has been tested on a bmw i3+kostal setup. are you fine with the non-kostal changes? |
@lewurm, to get this PR approved more quickly, please remove the changes that are done in the battery file |
@lewurm , could you use the new logging method that the rest of the code uses? That would enable easier debugging, no need for the USB cable anymore! |
a278d73
to
61f5bc6
Compare
Thanks! Please have another look @lenvm and @dalathegreat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. Hope this helps Kostal!
Example: