New Scale Factor not used immeditaley, old one used instead

[Sputnik24]

Describe the bug
I have a Fronius Symo Gen24 with Fronius SmartMeter and reading all important data via modbus TCP. This is set as int+SF and I'm using the formula and SF feature to calculate the real value out of raw+SF. I transferred the values of interest from an Excel file provided by Fronius to CSV and imported it to the adapter Holding Registers. For most values, the SF has a higher address and is after the int value in the register, for a few it's the other way round.

Problem: When the SF is changed, the adapter uses the old SF to calculate the real value out of the int value. On the next poll, when the SF is not changed again, the calculated value is correct. But whenever the SF is changed, the calculated values at the poll, where the SF changed, is wrong my one order of magnitude. See the screenshots for example.

Screenshots & Logfiles My settings:

Part of the Holding Registers with both examples, SF after (e.g. 40083) or before (e.g. 40274) the int value.

Example data from influxdb with the jumps marked

Corresponding SF logged with influxdb, you see that the jumps above are exactly at the time, where SF changed.

Both values above in Grafana. Even worse, if the SF changes only for one poll and than back, two values in sequence are wrong (example 1 on Grafana plot).

Versions:

• Adapter version: 3.3.0
• JS-Controller version: 3.2.16
• Node version: 12.20.1
• Operating system: Docker in Synology DS916+ with DSM 6.2.4-25556

Additional context
Problem is not observed for the values where SF is before int value. Maybe, a workaround would be to put all SF first, but how can I change the order of the Holding Registers settings without loosing the datapoints created?

[Sputnik24]

Update: Moving the SFs above the int values, did not solve the problem.

[Apollon77]

Please enable debug log and post a debug log of such an effect

[Sputnik24]

Thank you. I reproduced the issue if debug logs.

Logs (including startup of adapter at the beginning) iobroker.2021-06-03.log.txt

Error: 2021-06-03 07:55:31.334 - debug: modbus.0 (2890) Poll holdingRegs DevID(200) address 40087 - 5 bytes 2021-06-03 07:55:31.354 - debug: modbus.0 (2890) Input Value = -14701 2021-06-03 07:55:31.354 - debug: modbus.0 (2890) Formula = x*Math.pow(10, sf['40091']) 2021-06-03 07:55:31.355 - debug: modbus.0 (2890) Scale factor value stored from address 40091 = -2

But at 07:55:31, -1470.1 is stored in the data point, instead of -147.01, though scaling factor is correct according to debug logs.

[Sputnik24]

New debug log together with influxdb log to confirm, that the wrong value is received: 2021-06-03 09:40:10.804 - debug: modbus.0 (9007) Input Value = -27059 2021-06-03 09:40:10.805 - debug: modbus.0 (9007) Formula = x*Math.pow(10, sf['40091']) 2021-06-03 09:40:10.805 - debug: modbus.0 (9007) Scale factor value stored from address 40091 = -1 2021-06-03 09:40:10.814 - debug: influxdb.0 (8992) Min-Delta reached modbus.0.holdingRegisters.200.40087_W, last-value=-331.9, new-value=-27059, ts=1622706010810

Scale factor before was 0.

[Sputnik24]

Me again. I extended the logging the following (I hardcoded reading current SF for one example) if (regs.config[n].formula) { adapter.log.debug('Input Value = ' + val); adapter.log.debug('Formula = ' + regs.config[n].formula); try { // calculate value from formula or report an error const func = new Function('x', 'sf', 'return ' + regs.config[n].formula); adapter.log.debug('Raw value: ' + val); val = func(val, scaleFactors[regs.deviceId]); adapter.log.debug('Scale facture currently stored: ' + scaleFactors[regs.deviceId]['40091']); adapter.log.debug('Value after formula: ' + val); val = Math.round(val * options.config.round) / options.config.round; adapter.log.debug('Final value: ' + val);

debug log: 2021-06-03 11:00:10.494 - debug: modbus.0 (4053) Input Value = -31809 2021-06-03 11:00:10.494 - debug: modbus.0 (4053) Formula = x*Math.pow(10, sf['40091']) 2021-06-03 11:00:10.495 - debug: modbus.0 (4053) Raw value: -31809 2021-06-03 11:00:10.495 - debug: modbus.0 (4053) Scale facture currently stored: 0 2021-06-03 11:00:10.496 - debug: modbus.0 (4053) Value after formula: -31809 2021-06-03 11:00:10.496 - debug: modbus.0 (4053) Final value: -31809 2021-06-03 11:00:10.497 - debug: modbus.0 (4053) Scale factor value stored from address 40091 = -1

It confirms that on change of SF, adapter uses the old value because it reads the new value later.

[Sputnik24]

Ich switch mal kurz auf deutsch.

Ich habe jetzt einen Workaround implementiert, der für mich das Problem größtenteils abfängt, aber nicht in allen Fällen. Aus meiner Sicht ist das aber ein grundsätzliches Problem und sicher eine Eigenheit von modbus und inf+SF.

Workaround: In der Funktion pollFloatBlock() geht er die for Schleife nun zweimal durch. Im ersten Schritt werden nur die regs.config Elemente verarbeitet, bei denen isScale true ist und entsprechend der scaleFactor gesetzt. In der zweiten Iteration wird der Rest gemacht, sodass die SFs vor der Berechnung in das sf array geschrieben werden.

Ursprünglich wollt ich das smarter machen, nämlich dass er den SF nicht aus dem Array holt, sondern direkt aus dem aktuellen Block, den er gelesen hat. Das funktioniert aber nur, wenn int und SF im gleichen Block sind, was aber auch für obigen Workaround gilt und damit sind wir auch schon beim grundsätzlichen Problem.

Aus meiner Sicht muss, damit int und SF immer synchron sind, beide mit dem gleichen Block eingelesen werden. Die smarte Lösung von oben funktioniert bei mir nicht, weil es einmal nicht der Fall ist. Hier werden zuerst alle SFs in einem Block gelesen und danach die ints. Das ist der einzige Fall, wo im HoldingRegister die SFs alle zusammen vor den ints kommen, weil mehrere ints den gleichen SF verwenden, in allen anderen Fällen kommt der SF immer nach dem int.

Jetzt könnte man meinen, ist ja gut, wenn die SF vor dem int als Block gelesen werden, dann werden diese zuerst aktualisiert. Dennoch kommt es vor, dass SF und int nicht synchron sind, weil bis zum lesen der ints sich der SF schon wieder geändert hat.

Wenn man es irgendwie hinbekommen würde, dass int + dazugehörige(r) SF in einem zusammenhängenden Block gelesen werden, könnte man den SF direkt aus dem Block zur Berechnung lesen. Wäre meiner Meinung nach die sauberste Lösung.

Was meinst du? Ich könnte meinen Workaround zumindest mal als Diskussionsbasis als pull request einstellen, wenn dir das hilft. Er macht zumindest nicht die bestehende Lösung kaputt, er minimiert nur das Problem.

[Apollon77]

@nkleber78, I think the logic for formulas and scalignwas prided in an PR from you ... Do you have an opinion here?

[Sputnik24]

Finally, I managed that all int and corresponding SFs are read in the same block. It was not the case for one part, where the int values for both strings (MPPT1 and 2) and battery charge and discharge shared the same SFs and the SFs came first. I checked the code to understand how the blocks are determined and found out, that in case there is a gap of more than 10 bytes between two addresses, a new block is generated. And that was the case above, as between the SFs and each block of MPPT1, etc. I had a gap of 15 bytes. I changed my holding register table, added the ID String of 8 bytes, no the gap is less than 10 bytes and everything is read in one single block and my workaround is working. The smarter solution would also work.

To sum up: I could identify and solve the issue on my side. I would like to rase a PR with this solution.

Additional infos:

Here you see the holding register of the situation described above. After adding the Input ID, the gap between the blocks is less than 10 bytes and everything is read within one block. int and SF are synchronous.

To get an idea of the severity of this problem, here you see a Grafana plot from this morning where above solution was not applied. You see the big jumps by about one order of magnitude in both directions which doesn't make sense physically.

Last but not least, we are not alone: https://community.openenergymonitor.org/t/logging-solaredge-inverter-data-using-modbus-over-tcp/16341 https://github.com/erikarenhill/solaredge-modbus-hass https://www.loxforum.com/forum/german/software-konfiguration-programm-und-visualisierung/50841-solaredge-wechselrichter-einbinden/page7

[nkleber78]

@Sputnik24 Thanks for your detailled analysis. I think there would another special case to be considered. That is the case where the maximum number of bytes per block is reached and because of physical limits the read must split into multiple blocks. This case we cannot cover. I see some ways to solve it

1. Do 2 reads, first all scale values and then directly after that the read of the data -> there is still a small risk of having a change in the scale factor in that time period which is might up to 1 or 2 sec depending on the speed of the device and the number of datapoints
2. Read first all data, buffer the data till everything is read, then evaluate the sf and later the formulas
3. Read the datablock and evaluate. Whenever the system uses a SF from an address, which is higher then the current one, the system either directly reading it from the current buffer or buffering the data to process after everything is read...

Solution 2 and 3 is quite tricky to handle. So for me the question is, if that is really needed to do. I have the feeling that even the option 1 should solve the issue for situations which are not too dynamic...

[Sputnik24]

@nkleber78 Thank you for your suggestions. In my opinion, int+SF must be read within one block, means when the read is split into multiple blocks, besides the 10 bytes gap and maximum number, this should be added to the split logic. In my opinion, it must be possible in all cases, otherwise the holding register is waste and wrongly implemented.

1. In my opinion, this solution will not work. This is what I have in the example above. First, all SFs are read and then the ints. And you see nicely in the Grafana plot, how often it fails.
2. Don't understand it. If int+SF are not read in the same block, in general, you have no chance to find out, if both are synchronous.
3. I would generally read it from the buffer, in case it is possible to ensure that int+SF are in the same block. But the sf array and updating it first before applying the formulas might be better as it covers also the cases where the SF cannot be read in the same block.

For me, option 1 would not work. I have currently option 3 running, defining the holding register table in a way, that the current split logic keeps int+SF within the same block. Of course, it would be a nice to have to do this automatically, or at least to give a warning to the user if it is not the case.

[nkleber78]

Btw. just found out why i did not see this issue. I am using float instead of int+sf. That brings the benefit that in all cases the scale factor needed is before the value to be scaled and less parameters which needs calculation compared to int+sf

[Sputnik24]

Btw. just found out why i did not see this issue. I am using float instead of int+sf. That brings the benefit that in all cases the scale factor needed is before the value to be scaled and less parameters which needs calculation compared to int+sf

This was also my first idea, but unfortunately, even in the float register, int+SF is used, anyway, e.g. for the example above :(

Honestly spoken, in my holy opinion the actual error is the dynamic SF which doesn't make sense technically and physically. int + static SF would do it. To say it quickly in German: Da hat sich am Holding Register ein Ingenieur ausgetobt, der von Physik und Programmierung keine Ahnung hat.

[nkleber78]

@Sputnik24 I fully agree with your opinion about that dynamic SF. That is strange here... It is true that int+sf is also used for floats, but there is at least the order always guaranteed to be before the value to scale. Splitting into blocks is another issue. Btw. i didnt understand correctly where you told "reading scale factors" I thought it was just done in the current block and not through all blocks. How big was the time gap between reading the SF and the rest of the data? Was it really just 1 or 2 seconds or was that still 2 cycles (e.g. 30sec in between)?

[Sputnik24]

Here you see my settings. The SF are read in the same poll with the int. I checked the debug logs: 2021-06-03 12:22:56.985 - debug: modbus.0 (8356) Poll holdingRegs DevID(1) address 40255 - 4 bytes -> the block with all SFs 2021-06-03 12:22:57.011 - debug: modbus.0 (8356) Poll holdingRegs DevID(1) address 40272 - 5 bytes -> the first block with int for MPPT1 ... 2021-06-03 12:22:57.112 - debug: modbus.0 (8356) Poll holdingRegs DevID(1) address 40332 - 27 bytes -> last block with int for Discharge

So, the time gap between reading SF and reading last block which uses this SF is ~130 ms in this example. To my information, the register is written every second, so the chance that int+SF changes between reading SF and reading int is not so small.

[nkleber78]

Ok, but then it is even not ensured in case you are reading all in one block because i am not sure if they have implemented a latching to ensure that nothing changes during the full holding register read... But for sure the risk is much lower. The question is then how to implement. Issue a warning in case the sF is not in the current block during execution on every cycle?

[nkleber78]

I will issue a ticket to Fronius. Maybe they are willing to solve the issue in any of the future releases...

[Sputnik24]

Good question. In my opinion modbus must ensure that int+SF are in sync if they are read in one block, otherwise you can throw this protocoll in the bin. To give one example, around 8 a.m. I changed my settings that int+SF are read in one block and you see that the issue did not occur so far.

So, maybe we can asume for now, that if int+SF are read in the same block, they are in sync (I will give an update tomorrow if it never occurred). Regarding ticket to Fronius: They implemented the sunspec which also used by other OEMs like SolarEdge.

First solution would be to update SF in the sf array first before applying the formulas, when looping through a block. This is what I have currently changed yesterday, and it's working so far. Second would be either how the split the blocks that int+corresponding SF are in the same block (and give a warning to the customer if it is technically not possible) or at least give a warning/hint to the customer that he/she must adjust the settings.

[Apollon77]

Great progress guys, thank you. I think you are way more deep in that topic then me ... I would be happy to get a PR

[nkleber78]

@Sputnik24 Could you try my fix and see if that is how you would expect? Thanks

[Sputnik24]

@nkleber78 I will do it tomorrow. Wanted to raise a PR myself, but you were faster :) and Visual Studio was on strike :( (to be honest it's my first PR)

At least, I checked your code, looks very similar to my suggestion. Thanks a lot for the warning, in addition. Questions:

Why do you process the formula part in the first iteration? Is a scale factore used together with formula? My first iteration is simply: for (let n = regBlock.startIndex; n < regBlock.endIndex; n++) { let id = regs.config[n].id; let val = common.extractValue(regs.config[n].type, regs.config[n].len, response.payload, regs.config[n].address - regBlock.start); // If this value is used as scale factor => store it if (regs.config[n].isScale) { scaleFactors[regs.deviceId][regs.config[n].address] = val; adapter.log.debug('Scale factor value stored from address ' + regs.config[n].address + ' = ' + scaleFactors[regs.deviceId][regs.config[n].address]); } }

Is there a reason for choosing 10 bytes as decision gap size to start a new block or not?

[nkleber78]

I thought that not processing the formula in this case without giving a warning is also not a good idea, therefor is simply processed the formula as well. Time wise it will not hurt as only the id's which are marked as sf are processed. Regarding the 10bytes gap: I think thats simply a guess where it make sense to split. But in my case anyhow multiple blocks must read as i have the first block at adress 40000 starting and the last dataset i need is on 40378. And at least on my side approx. 120 registers is the maximum which can be read at once.

[Sputnik24]

I applied your commit and removed one of the IDs from the holding register to check if the warning is working: 2021-06-05 08:31:46.722 - debug: modbus.0 (4515) Poll holdingRegs DevID(1) address 40332 - 27 bytes 2021-06-05 08:31:46.742 - debug: modbus.0 (4515) Input Value = 0 2021-06-05 08:31:46.743 - debug: modbus.0 (4515) Formula = x*Math.pow(10, sf['40255']) 2021-06-05 08:31:46.743 - debug: modbus.0 (4515) Scalefactor adress is = 40255 2021-06-05 08:31:46.743 - debug: modbus.0 (4515) Scalefactor adress is inside current read range = false 2021-06-05 08:31:46.743 - warn: modbus.0 (4515) The current range for reading the values was from adress 40332 up to adress 40359! 2021-06-05 08:31:46.743 - warn: modbus.0 (4515) Please make sure to configure the read process that both adresses are read in the same block! 2021-06-05 08:31:46.743 - warn: modbus.0 (4515) The used scaleFactor from address 40255 is not inside the same read block as the parameter on address 40332

Here, we have the data of the last 24 hours since I use my workaround which works basically same as yours.

The jumps did not occur within the last 24 hours, though scaling factor changed a lot.

I will let your commit run and give an update by tomorrow.

All in all, thanks a lot for your help, discussion and solution. For sure, we can think about improvements, e.g. to inform the user about int+SF not beeing in the same block when saving the register config. Or to improve the splitting in a way, that this criterion is also covered. But this might not be so easy.

[nkleber78]

@Sputnik24 Hope that this fix works fine. If you have time, then it would perfect if you could think about a process of defining the blocks automatically in a way that the scaling factors are in the same block as the formulas. In addition checking this behaviour in the admin and give a hint to the user how to solve it (e.g. needed block sizes ...)

[Sputnik24]

@nkleber78 With your commit issue did not occur within the last 24 hours though SF changed a lot. From my side, you have the go for a PR

Regarding taking int+SF in same block into account of splitting the request, I'm already thinking about, but I guess it's not an easy task - even as I only know my case and the holding register of Fronius (which schould basically the same for all which are using the sunspec). My general thoughts:

When it loops through the config, it should check if the formula contains sf or if the register itself is marked as sf. In case this is true, it should not split the block until the corresponding sf or all registers using this sf are found (so it should overrule the 10 byte criterion, sure, the maxBlock criterion always wins). If sf comes after the int, this should be easy (assuming that all ints using the sf have lower addresses than the sf itself). In case, sf comes first and than all int using it, it's a little bit tricky, as you need to look for the int using this sf with the highest address. An additional challenge are cases, where the int with sf or the sf itself are part of a block before (gap lower than 10 bytes), then, the block cannot be split because sf was found, but then the block gets too big. In those cases the block before shall be split though the 10 bytes criterion did not match.

I'm thinking about, if three criterions for splitting would be better in the following priority: maxBlock size, int+SF in same block, minimize number of blocks. Maybe, this is a task for a recursive solution.

[nkleber78]

@Sputnik24 I think in general your idea is good. I would might do it in the following way

• Loop through the config and calculate for each sf the upper and lower limit where it is used. E.g. you find a formula which uses sf 4005, then store to the sf4005 this value either as minimum or maximum depending if already a value set or not. In case both values are currently not set, then both are set to 4005
• After that loop is finished, you can calculate the possible blocks according to this limits. If it is not possible to create a valid config just raise a warning after it was split as good as possible. Maybe also give a hint of changing the maxBlockSize to a certain value...
• The split with the 10 bytes i would also see as the lowest priority

[GermanBluefox]

Fixed with https://github.com/ioBroker/ioBroker.modbus/pull/128