-
Notifications
You must be signed in to change notification settings - Fork 40
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
Improve docs and add examples #10
Improve docs and add examples #10
Conversation
…os, extend comments in example code, relates to #9
I'm not sure, but I think I gave the feedback in the wrong place. Maybe it should be here and not as this reply on the #9. If here is the correct place to the feedback, please, let me know that I can to move from there to here. Thank you so much. |
Here the all errors found in my tests using the PR #10:
1. On the Slave implementation docs show "This requires the modules submodule to be cloned..", but that URL is broken. 2. On the Modbus TCP Client example I think that line 42 need to be inside the 3. The Modbus TCP Client example do not accept more than 4.
However I tested from another Ps 1: Take a look on the name 5.
However from the 6. On the register_definition ->
7. On the register_definition ->
8. On the register_definition ->
Thank you very much! |
…he corresponding example
First and second mentioned topic is fixed without ticket
That is true. According MicroPython Socket docs the
as I only had one connection so far, I did not see this issue. To be fixed in #11
Fixed by #13 the default value of parameter |
…, 1, False, True, 0x0, 0xFF00, resolve #14
Hello @brainelectronics Thank you to fix the bugs and create new issue tickets to solve. I can confirm that bugs marked as solved/completed are working now! I found more 1. I believe that return (
2. Error when trying to write multiple coils ( I not found examples on the docs about how is the correct way to write multiple coils, so I get a example from here
3. Error when trying to write multiple registers (
Thank you very much! Edit: This problem happens in both: RTU and TCP. |
Hello @brainelectronics Could you to please take a look if is easy to fix that problems to write Thank you in advance! |
@beyonlo I'll have a look at it on the weekend again |
Hey @brainelectronics did you have any success at the weekend? :) Thank you so much! |
I had no time for this repo on the last weekend. The code in these repos is not created in my day job. But I have a comment on your issue with reading a single coil.
See https://modbus.org/docs/Modbus_Application_Protocol_V1_1b3.pdf Page 11 So returning a list of 8 true or false elements is following the standard. |
Hello @brainelectronics I'm sorry to bore you. Please, don't get me wrong. Take your time to fix that. About the issue #12, reading a single coil you are right, it is following the standard. The return is always multiple of eight, and the next sentence of Page 11 confirm that:
Thank you and have a nice week. |
Hi @brainelectronics Just a small error in the docs: on the Master implementation docs, on the
On the I saw that you created the issue #16 ( |
@beyonlo thank you really for your patience! I believe your comments and reports are really valuable for this lib! Keep reporting 😀 If it's a new thing to solve, directly create a new issue. Otherwise, if it is related to this PR, comment here so I can resolve it within this PR. I'll extract code of the general helpers repo into this one to be able to really test it on my hardware. |
1 similar comment
@beyonlo thank you really for your patience! I believe your comments and reports are really valuable for this lib! Keep reporting 😀 If it's a new thing to solve, directly create a new issue. Otherwise, if it is related to this PR, comment here so I can resolve it within this PR. I'll extract code of the general helpers repo into this one to be able to really test it on my hardware. |
@brainelectronics You are welcome! I'm very grateful to helping, at least, testing this lib.
I agree!
I'm not a Python expert, but I did a take a look in that repo, and I think that you will adapt it to create/use as a new framework (#16) to work testing and fixing the bugs.
No problem. At least, even with few time, there is consistency - that is very important :) When you have a new version/PR (even not finished) let me know that I can to do all tests again! |
Hello @brainelectronics How are you? I trying to choose the right words here to you do not get me wrong. English is not my native language, so something can be not perfect :) I would like to know if do you have plans to fix that all problems in this lib and have it 100% following the I'm developing a simple project with I'm trying to figure out how I can help you in this process and improve this amazing ModBus library. Unfortunately I have no acknowledge enough in development to help you with Thank you very much for your attention! |
Hi @beyonlo I'm also thinking about a good way for documentation like RTD but I would need to invest some time to get familiar with it. |
Hello @brainelectronics This is a wonderful news! I'm so happy :) Thank you very much for your effort! |
#19 provides possibility to test release candidates, see https://github.com/brainelectronics/micropython-package-template#test-version import upip
# overwrite index_urls to only take artifacts from test.pypi.org
upip.index_urls = ['https://test.pypi.org/pypi']
upip.install('micropython-modbus') |
Hello @brainelectronics I did a simple test on the new release 1.1.0. I dowloaded it from the 1.1.0 tag instead to use upip. I would like just to notice you that some bugs that was already fixed during the PR #10 are back in this new release. Maybe you already know that, or just started to works in the new version using the original 1.0.0 instead from that PR. Ps: as I checked in the 1.1.0 changelog that you still not fixed other bugs, I didn't tried to test other things. Thank you very much! |
@beyonlo I startet from the latest version on develop which did of course not contain the fixes and changes of this PR. I'll later pull the now updated develop to create a 1.1.1 release. I'll also finish my test setup within the next few hours to be sure everything is really backwards compatible. |
@beyonlo this PR is updated so far. I'll merge after your approval/review and continue with the next tickets and fixes |
My understand is that I need to test this PR (named as 1.1.1) just what are described on the changelog.md in the version 1.1.1, listed as Fixed. I will to test tomorrow, because I'm going to sleep now :) I would like to notice you that the link 1.1.1 inside changelog.md is broken. I clicked in that 1.1.1 to download to test, but I understand that this PR already is the 1.1.1. Sorry for me to confirming above somethings that maybe are obvious for you, but not for me! If I'm missing some understand, please, tell me. Tomorrow I will post here the tests results :) |
Your of course absolutely right with the broken link in the changelog. The tag Right now you could install the package like this import upip
# overwrite index_urls to only take artifacts from test.pypi.org
upip.index_urls = ['https://test.pypi.org/pypi']
upip.install('micropython-modbus') That would install the latest version micropython-modbus 1.1.1rc3.dev10 on your board. |
Tests done on the Note: now the new version need the Tests results Short asnwer: all passed Long asnwer: details of tests below For all tests was used this Slave TCP:
1.1.1 - 2022-11-09, Fixed: Bugfix number 1: Default value of Master TCP:
Bugfix number 2: Missing function docstring added to Nothing to test. Bugfix number 3: Master TCP:
|
Test report, part 2: Important note: just to remember you, there are many bugs reported in this PR that was not fixed yet, and was not created separated tickets for them. If you prefer, let me know that I can to open issues for them. On the Bugfix number 3 I forgot to test the Short answer: all passed Long answer: details below
|
Excellent, I agree! :) Tomorrow I will open an individual issue for each bug reported in this PR that is not fixed yet. Thank you very much! |
Done, all issues created. When you have new versions, just let me know that I can to do all tests. Note 1: The last test I did just using Note 2: My mode to test this lib is using this way: ModBus RTU: ESP32_1 Thank you so much! :) |
Related to #8 and #9
Changes