drivers/nutdrv_qx.c: improve Armac subdriver#2005
Merged
jimklimov merged 7 commits intonetworkupstools:masterfrom Aug 17, 2023
blaa:master
Merged
drivers/nutdrv_qx.c: improve Armac subdriver#2005jimklimov merged 7 commits intonetworkupstools:masterfrom blaa:master
jimklimov merged 7 commits intonetworkupstools:masterfrom
blaa:master
Conversation
jimklimov
requested changes
Jul 31, 2023
jimklimov
added
bug
need testing
jimklimov
reviewed
Aug 1, 2023
Based on a debug output from a newer device (*/PF1) we've improved understanding on how: - those devices encode the length of a chunk of data. - how the end of transmission can be marked / detected. Changed: - Empty buffer before sending command to clear any residual data. - Detect end of message by end of line character \r (0x0d). - Refactor "6" into a READ_SIZE constant. - Limit bytes_available nibble to available READ_SIZE. Signed-off-by: Tomasz bla Fortuna <bla@thera.be>
Author
|
Trace -DDDD from a new UPS: https://pastebin.com/2dgc2X0Z (the debug ASCII output is a bit changed but that is not in PR as it was work in progress) Debug output from my UPS (older?) works as well: So - after the current changes both known versions seems to work ok as far as I can tell now. |
jimklimov
reviewed
Aug 3, 2023
Member
|
CC @bdkacz @desertwitch @convicte : Cheers, would any of you guys please be able to build and test https://github.com/blaa/nut/tree/master to see if it supports all of your armac-talking devices? |
Fix debug printout Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
blaa
commented
Aug 4, 2023
Author
I've tested this code with @convicte already. He seems to be staying with Armac so we can do a final test after merge (maybe after release, unsure when it's planned). I can do more tests with mine UPS after 21 of August. Feel free to freeze this change until that time. bdkacz didn't respond earlier so I guess we can't really hope for him to test that. ;) |
Member
|
In the last round, something new: I guess it should after all portably be a macro like |
Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
Contributor
|
Interesting, I've built this without any warnings or errors on Slackware v15 for @convicte to test on his UNRAID installation (don't have an Armac UPS myself). Waiting to hear back from him. |
Member
|
Yeah, the other 138 build matrix combos were also fine with this, just a couple squealed ;) These two in https://ci.networkupstools.org/job/nut/job/nut/job/PR-2005/8/ were:
So essentially clang7, cstd=gnu, and the warnings its analyzer emits. |
Author
|
I have one more person with some weird new configuration (Vultech). I'll try to support it via this latest commit but will have to make some tests with him. It's probably better to wait a bit and make him work as well. |
|
Hi all! @blaa I have wanted to check if there was any progress on #640 , and well, even if with some delay, I am still alive :D . While I've to check #640 if supports really my ups (I have to compile nutdrv_qx for openwrt, where the ups is attached), I have noticed youre last comment here. Well, my ups is specifically a Vultech, an UPS1000VA-PURE/UPS1500VA-PURE https://www.vultech.it/it/gruppi-di-continuita/915-ups-1500va-pure-line-interactive-onda-sinusoidale-pura-gruppo-di-continuit%C3%A0-8052780300902.html So, at this point, I will wait for another commit :D , and ping me if you need something for that configuration too. |
Author
|
@mibofra you can help by pasting nutdrv_qx output with subdriver armac and "-DDDD" debugging level. It's great time to push it a bit forward as I'm still on vacation and can do some night-time hacking. I'm far from my UPS though. Something like this: With each new trace our understanding of how this transmission works improves. |
Author
|
We've got an "almost" elegant solution by treating NULL bytes within the status bits as zeroes on Vultech V2000. This makes the UPS answer with another 6 bytes and eventually we reach \r byte and number of status bits matches the requirements of Qx / Megatec protocol. It might've worked previously because there was no 0x00 detection and null byte was just included within the output. Still, it worked by accident AFAIK. I'll publish code changes later. Maybe adding a doc file with all possible transfers for this drivers makes some sense. I've even coded a testcase where I emulate USB locally, but it's just a hack on the driver that requires USB device to connect to SOMETHING. My knowledge here is too weak to create a proper automatic test. |
Member
|
Sounds great, thanks! |
This UPS seems to use null bytes within status bits. This might mean "unsupported". We will treat them as zeroes. Signed-off-by: Tomasz bla Fortuna <bla@thera.be>
Member
|
Thanks again! Posted some comments about the wording and markup of the new text file, but maybe posted that to commit and not PR :) see: 4b105f8 |
Member
|
Also, it may be worth listing the devices in |
Member
|
Thanks! Are there any lingering ideas or can this be merged now? ;) I for one wonder if the protocol description makes better sense (and would be easier found) among https://github.com/networkupstools/nut-website/tree/master/protocols after all... |
Author
|
Not much new ideas. After 21.08 I should have access to my UPS and can conduct some more testing then. @mibofra could try this with his UPS for some additional testdata. As for the docs - unsure. TBH, whatever you think is fine. It would be cool if you could move it though if you want it there. ;) I've never taken a look at this other repo so the findability might not be that great (I usually vote for monorepos). |
Member
|
Oh, well. For this one the point is a bit moot - found there is a The nut-website ones are more about formal and exhaustive protocol definitions, whether from vendors or reverse-engineering... |
Author
|
If you prefer I can merge this new file with nutdrv_qx-subdrivers.txt as a subsection maybe? |
Member
|
Thinking of it - yes, please. It would be easier than linking to it from other docs and makefiles and missing half. :) And would be easier to find for those who'd need it, too :) |
Document various possible transmissions. Might be required to construct testcases one day - adding new UPS shouldn't cause regressions in older ones. Signed-off-by: Tomasz Fortuna <bla@thera.be>
…te the dictionary [#2005] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
Member
|
Thanks for the improvements and for the patience! :) |
Member
|
For next times, keep in mind it would be more sustainable for you to start a PR from some feature branch, not master. Especially when it is predictable that it won't be a five-minute merge win :) |
Author
|
I'm glad we'd opportunity to improve this driver. Now lets see what new bugs or UPSes appear. ;) I'm more used to a Bitbucket PR model (we have it at work). I'll have to look up how it usually works on github. For me the small problem is using two repositories (two remotes because of a fork), so I assume you mean a feature branch on the nut repository - if that's possible then certainly would be easier. Cheers and thanks for your work here. ;) |
Member
|
No, it's usually still a developer's fork (hard to juggle permissions in same upstream repo). So you fork NUT on github (once, done), then |
Author
|
It works on my UPS still ok - finally tested it. |
Member
|
Thanks for the contribution, and time, and diligence :) PS: If you have a chance to chime in, issue 1987 was last waiting for suggestions about contributing a (probably) |
|
Hi all, hi @blaa . Well, I had time now, so I've logged some debug from the driver today. I hope it will be still useful, even fi this pr was already merged. I was lazy, so I've compiled nut simply on my workstation and attached the ups for debug. Details
hoping it will be usefull anyway (or provides an "ok, it is working!" sign) :D . |
Author
|
Indeed from the log - it looks like it's working just fine. So for future reference that was Vultech UPS1000VA-PURE/UPS1500VA-PURE UPS. In case of problems - drop us a note. Scheduling a test for example should work too. Changing beeper setting - maybe not. |
jimklimov
added
the
LEDE/*WRT embedded Linux firmwares
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on a debug output from a newer device (*/PF1) we've improved understanding on how:
Related to:
#2003
#1978
Closes #1978
I'll test this changes for "old" devices in september. Maybe earlier.