-
-
Notifications
You must be signed in to change notification settings - Fork 81
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
Plans for v3.0 - discussion #19
Comments
Hello Jack. I am using the library in some embedded targets. https://resources.altium.com/p/demand-digital-manufacturing-and-32-bit-microcontroller-everyone In my use case, I am using the experimental callback mechanism as registers in use are no contiguous. In one case, I have a register that is writable (a special parameter) only if another register is set to a specific value. What I would really like is some more control in the callback to return an error to flag a return message of "Illegal Function" or "Illegal Data". When the callback is called with "MODBUS_REGQ_W_CHECK", it would be nice of the "value" field was set to what was going to be written. That way I could reject based upon specific values of the register, not just the address. It is very possible that this code exists now and I am not properly using the calllback. Either way, thanks for the library. I really like how the physical layer is separated. It made integration much easier as compared to some of the other libraries. We did some extensive testing/fuzzing on it and it works well. |
@ehughes Hello and thank you for your kind words. I'm very happy to hear that the library serves you well. I really like the idea of register value being passed to callback function during write protection check - I've never thought of that. Sadly, in the current version, the value passed to the callback during access check is always set to 0. I suppose it's worth mentioning that all kinds of interdependent registers introduce some problems when it comes to writing multiple values at once. Perhaps the safest approach is to disable functions 16 and 15 on such registers. That means that the callback function should also accept Modbus function ID as a parameter. Giving the user more control over what kind of error frame is generated is a good idea too. I suppose we could have the callback return Thank you for your insight. I've updated the TODO list. |
Hi Jack. I am using the library, too. Initially I tried to use dynamic memory but for unknown reasons on atxmega32a4 I got some mysterious behaviors. However, by changing that to static memory it became fine. |
Hi, It's because I wanted to avoid code repetition. The logic behind reading discrete inputs and coils is exactly the same, only the source array is different.
I've never worked with ATXmegas before, but it's good to hear that you managed to solve that issue. This seems to prove the point that memory management should be left to the user in v3.0. |
Hi Jack! Thanks for the library, I really like the optimizations and how it's written. I use your library on a dsPIC33CK and, after some time, I was able to make it work. Here my two cents:
With respect to issue number 3, I'd say that it probably took me more time than needed, because you already gave me the solution, but I was too blind to see it: you had already patched a misalignment issue on the coil library, I just needed to copy it on the registers. I'll may send a PR with this fix, but I haven't looked at the parts I don't use (dynamic allocation and modbus master) so, for now, I just post here the solution for those that, like me, have memory issues with static allocation: just copy these lines in the sregs.c file, from line 120 to 126. THIS
BECOMES THIS
Cheers! |
Hi! Thanks for your very detailed insight - I appreciate it very much. 👍
It's seems that perhaps CMake wasn't the right choice for this project. A lot of people seem to be having trouble with integrating the library into their projects because of it. I've never given it much thought, but now it struck me - maybe we should go header only? This seems to be the best choice for a platform and environment agnostic library. This may also give the compiler a better chance to do some optimizations. Configuration would be a bit simpler too - the config file could be replaced with just a bunch of macros defined before including the library: #define LIGHTMODBUS_F01S
#define LIGHTMODBUS_BIG_ENDIAN
...
#define LIGHTMODBUS_IMPL
#include "lightmodbus.h" That obviously doesn't stop us from having a CMake config allowing anyone who uses it to simply write
Very good point.
I'm sorry to hear that. The fault lies definitely on my side here. I'll fix that right away. What I meant by "leaving memory allocation to the user" was that instead of having the library allocate memory or define static buffers for you, you just need to provide it a pointer to some buffer you allocated. I think that would significantly simplify the design and allow me to clean up the code from all the complicated preprocessor stuff. The library code will still have to be able to safely read words from the provided memory region, though. EDIT: Perhaps it's a good idea to stop relying on
I've never used CException, but, funnily enough, I once wrote similar library just for fun - cex. Having 'real' exceptions would be really handy for dealing with errors. I'm quite worried about mixing Thanks and please let me know you think of all that. |
Hello Jack: One thing I am looking at is Modbus TCP. I am doing some work on a cellular application (NRF9160) and will be doing both ends. The device will be a Modbus-RTU device and be a Modbus TCP master. For me, simply having helper functions to put on the TCP header would be enough. This functionality might exist but I thought I would bring it up. |
Also, I am using cmake (but not very skilled). Some sort of integration work to make including the cmake code from your library would be helpful. Right now I copy/paste sections into my own cmakelists.txt |
Hello, I've never worked with Modbus TCP so liblightmodbus doesn't currently support it. However, if converting frames from RTU to TCP is just a matter of prepending a proper header I think it could be implemented as an extension in future versions. I'd consider that something of a secondary priority, though. I agree, for people already using CMake a config for "including" the library would be really helpful. It could be as simple as defining a single interface target with proper include directories if I transform the library to header-only. |
Yes, Modbus TCP is simply a different header (and no CRC at the end). |
The Modbus TCP header seems to much simpler than I anticipated. I don't see any reasons why not to implement it in the core library functionality :) |
I mostly write for embedded, in C, and I've never used CMake for it. I would personally never use it, but maybe someone out there is finding it useful... I find myself closer to the "header" idea, because this would be the easiest way for everyone to integrate it. In my everyday life, I also define a good part of the "configuration constants" while calling the compiler: this is because I use the same code for different chips, but MplabX allows me to set different configurations with different
If you think that would ease up the job of the library.. yeah, why not? On the other hand, I really liked the idea of having it like it is right now because once it works for a chip, it works for every different program on it. Let's say: I'm not adding new bugs every time I start a new project, because I just need to say "hey I want the buffer this long" and the tests are already done. But it's just my two cents
I guess that we could add a test that checks whether or not the memory alignement causes issues and warn the user about it, with possible different solutions. On the other hand, in my experience, if we write things one byte at the time we shouldn't have issues.
haha cool! You might be right, I have not tested enough to feel safe about that. |
The only reason I decided to use CMake was to replace an awful "custom build system" written in bash. While it's really a fantastic tool for building bigger projects that depend on other CMake libraries, I agree that for embedded systems it's an overkill. Changing liblightmodbus to header-only seems to be settled then :)
You're right. What about a custom allocator callback like this It's only a rough idea, though. I literally just came up with this when replying.
What kind of checks do you mean? I think that reading one byte at a time is the best possible solution, because of it's simplicity. Maybe add just a couple of helper functions for reading words and dealing with endianness, and that would be it. |
Hi, I just wanted to let you know that I pushed some changes to the The main changes are:
Also, I wanted to ask you whether you've actually ever found Please let me know what you think :) |
I have a little update, just so you know that something is still happening. I think that at this point most of the hard work and thinking is done. After a bit of struggling to find the best possible solutions and a couple of dilemmas, I successfully managed to implement all of the biggest and most important changes to the library. New changes:
There are still a couple things to do, though:
EDIT: You can preview docs for the new version here: https://jacajack.github.io/liblightmodbus/v3.0/ |
Hi there, I have another update. I think v3.0 is pretty much ready. As you may have noticed, I already published some release candidates here and put up an announcement about the new version in the readme. The new release should be published at the end of this month. 🚀 In the meantime, I'm planning to focus mainly on more extensive fuzzing. I don't think I will be making any significant changes to the code. Please feel free to try out the new version and let me know if anything bothers you. You can also take a look at the new examples and the documentation. Thank you for all the feedback and ideas I got so far. |
Hi!
It's been over 2 years since the v2.0 release, the last major update to liblightmodbus. I think that many things can be still improved and cleaned up. Since there are apparently quite a few people using this library, I feel that I owe you all a next release. But before I break anything, I'd like to hear your suggestions and ideas.
An overview of currently planned changes can be found here. If you have anything to suggest, please write a new comment below or open a new issue.
Please let me know what you think.
The text was updated successfully, but these errors were encountered: