-
Notifications
You must be signed in to change notification settings - Fork 22
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
💅 Final polishing, and better understanding of how the demo works #92
Comments
wrt max current, it is set via node_red using which invokes
and is less than
it then sets it on the BSP and signals.
This is the max, so it is clearly not the source of the 48
and it is clearly published from
so where is it invoked from? It is initially invoked the giant
and the energy manager uses https://github.com/EVerest/everest-core/blob/828072742f816d74d44d55fdf13d01c8fbecd449/modules/EvseManager/energy_grid/energyImpl.cpp#L353
But the only energy that I see is
Need to add some logs here to see what is going on. |
In parallel, let's figure out where the default composite schedule comes from. The composite schedules that we use come from
which calls
Bingo! Here's where the defaults are set
and looking at the |
Going back to the other limits, they are indeed set from the modules/EnergyNode/energy_grid/energyImpl.cpp
or
The default config has it at 32, but this variable is set to 40 in our config This has been true since the very first commit Let's add some logs and figure out what is going on... |
rooting around within the
Let's see where they are coming from. Bingo! They come from the hardware (aka the BSP), and in our simulator, the BSP is the Note also that the API publishes these values once a minute Let's get some more logs, and then make sure that we set all the locations to 32 and see if everything is more consistent then! |
Logs confirm that the limits are coming from the hardware capabilities. I am not seeing the call to the fuse API happen yet
|
double checked today morning with a fresh pair of eyes, and can confirm that:
Maybe that is OK - the CSMS thinks that the station can have 48 A although the station has a hardware limit of 32, and then the limit should be 32. But shouldn't that 32 be reflected as an external constraint then? Need to discuss with the community @the-bay-kay and @Abby-Wheelis. When plugged in, the kW also seems to reflect the 48 (240 * 48 = 11.5kw) which is wrong since we can only provide 32A at the hardware level. Going to investigate that next... |
It seems there are a few different issues occurring in the powercurve generation. For one, we're not generating any of the previews, because the
So, we can assume our
... and we fail to find the script entirely. Running |
Will update this comment as I find more details, but wanted to link to my findings on the PowerMeter issues here before I forget (this thread was originally concerned with piping through the ChargingProfileSchedules, but the findings concerning JSYetiSimulator's PWM implementation should remain relevant). |
Ah maybe I missed that while porting over the changes. In the Dockerfile is fine, there were just soooo many patches for the manager that I thought it would be tidier to put them into scripts. But the nodered Dockerfile is fairly simple now. |
Some updates on the Node-RED changes!
Video of existing changes included in the cut below -- Changes to Node-RED Democompressed_SECCSchedule.mp4 |
So I finally figured out the value in the gauge. And it is in fact tied back to the 48 and some confusion around naming. Starting with basic electrical engineering, AC, unlike DC has phases. In the US, we typically use 3-phase charging. The total power is drawn across the three phases. Most chargers in the US apparently use 40-48 amps on a 240v line, giving us around 11520W or 11.5 kw. While supplying 48 A of current, the total current is split between three phases, giving us ~ 16 A per phase. That is the L1, L2, L3 current/voltage that we see in the powermeter output logs. The demo/simulator seems to have some inconsistencies between values. Going forward, we need to decide:
The inconsistencies are:
So those are internally consistent and make sense. The 32A comes from a different location - the hardware capabilities of the charger, defined at It appears, though, that this capability is not specified at the phase level, but is combined. But the slider on the UI is at the phase level. That makes the slider confusing. We see it at 32A, but nothing starts happening to the power drawn gauge until we get down below 16A. So how can we fix this for the demo?
We should then go over all this and understand which limit is being specified where, and unify them for readability. Finally, the simulation settings or the power meter are defined in
Note also that we can see the delivered power in greater detail in the "Debug" screen; see screenshots below.
|
I also double-checked, and the Yeti reference hardware has a cap of 16A.
That's another option 😄 |
I changed the value in the simulator and verified that it is getting invoked by adding logs, But the max limit is not changing. |
Hardcoding also does not work. However, I do notice
This is indeed the reason
Let's try to set the config as well! |
Ah it is set to 32 by default. Let's override...
|
Now we see a limit of 48, but it is then dialed down to 40, presumably as part of the fuse value. Setting that to 48 as well.
|
It looks like the limits applied to the JsYetiSimulator are on a per-phase basis, although limits applied elsewhere are not. This makes things very confusing. We will fix this by configuring everything to a per-phase limit of 16A Please see EVerest#92 (comment) to EVerest#92 (comment) Locations changed: - config - JsYetiSimulator - SmartCharging OCPP defaults Also fix the disable_iso_tls patch to not have a starting `/` Add a new patch file to enable limit logging but don't enable it by default. This may help future efforts to debug this. Signed-off-by: Shankari <[email protected]>
Consistent with EVerest#92 (comment) History starts at: EVerest#92 (comment) Also show dots in the chart Also convert the time range to/from local time so it makes sense Signed-off-by: Shankari <[email protected]>
wrt #92 (comment) @the-bay-kay can you pull my node-red changes, merge them and submit a PR with your changes? I think we can be GtG then! |
Awesome! I'll leave There were no merge conflicts, so I'm assuming it should be smooth sailing -- I'll create the PR once I know I've run some tests and confirm things are working! |
@the-bay-kay great! I think it is particularly important to ensure that, when we specify a power delivery curve as part of the power delivery req/res, that the charging "gauge" reflects that. If it doesn't, you may need to take a look at I think this is really and truly the last piece for the demo. I don't plan to investigate the other issues in the time left. |
No need to do this; all images (except the manager) are built by the CI and pushed. We should be using pre-built images, and only pre-built images in the demo.
The manager is not built by the CI due to lack of resources. So there will always be a ~ 30 min lag while I build and push it locally. Just try again in a bit.... |
@the-bay-kay just checking in on where this is. would be great if you could check in what you have so I can do a dry-run today |
I'm done with the Node-RED cleanup -- there were some trailing issues with how a fresh-clone behaved, such as the graph default, graph editing behavior, and setting our default simulation. Feel free to check out the existing changes in the draft PR here, or just by pulling my fork here (you'll need to tweak As for the powermeter debugging -- it seems that our local limits are set up within energyImpl.cpp here, with the |
@the-bay-kay Let's get the node-red part checked-in and pushed, and we can have a short conversation around what to expect for the second around what exactly is not working and what we need to do about it. I thought we had tested everything before... |
It looks like the limits applied to the JsYetiSimulator are on a per-phase basis, although limits applied elsewhere are not. This makes things very confusing. We will fix this by configuring everything to a per-phase limit of 16A Please see EVerest#92 (comment) to EVerest#92 (comment) Locations changed: - config - JsYetiSimulator - SmartCharging OCPP defaults Also fix the disable_iso_tls patch to not have a starting `/` Add a new patch file to enable limit logging but don't enable it by default. This may help future efforts to debug this. Signed-off-by: Shankari <[email protected]> Signed-off-by: the-bay-kay <[email protected]>
Consistent with EVerest#92 (comment) History starts at: EVerest#92 (comment) Also show dots in the chart Also convert the time range to/from local time so it makes sense Signed-off-by: Shankari <[email protected]> Signed-off-by: the-bay-kay <[email protected]>
Those changes are pushed and checked in! As for getting the ChargingProfiles to reflect on the powermeter -- my plan is to store the ChargingProfile in the I've added the ChargingProfile to the v2g context, and am attempting to store it in |
@the-bay-kay can you clarify what problem you are trying to fix? The Charging Profile is reflected in the power drawn. After my changes to drop the limit down to 16A, changing the charging profile does change the power gauge. Also, although I can build the node-red changes locally, I am not able to build them remotely. I really don't like docker commit because, as you saw, it makes it hard to track the changes required. |
Couple of issues with the current implementation:
|
- Do not throttle the SASchedule from the EV based on departure time, only transmit the composite pmax schedule. The EV will deareate based on departure time - Show the values in the current power delivery request instead of the progress towards the eamount so that we can see the impact of curtailing the pmax - Pass in the pmax to the power curve computation algo, although it is currently a NOP This fixes: EVerest#92 (comment) Signed-off-by: Shankari <[email protected]>
I think the demo is in pretty good shape for the kind of high-level, superficial exploration that we can do in 15 mins. But there are still some inconsistencies I found that I would like to take the time to fix. Some of these may just be my lack of understanding (e.g. #90), but then I would like to deepen my understanding to be ready for questions during the office hours.
I will keep a summar in the description and the details below.
The text was updated successfully, but these errors were encountered: