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

Win32 problems #187

Closed
vgrin opened this Issue Jan 8, 2014 · 12 comments

Comments

Projects
None yet
4 participants
@vgrin

vgrin commented Jan 8, 2014

Hi,
I have found some problem in Win32 implementation:

  1. The function

int modbus_tcp_pi_listen(modbus_t *ctx, int nb_connection)

requires the same _modbus_tcp_init_win32 call as modbus_tcp_listen, _modbus_tcp_pi_connect and _modbus_tcp_connect functions

modbus-tcp.c line 521

int modbus_tcp_pi_listen(modbus_t *ctx, int nb_connection)
{
int rc;
struct addrinfo *ai_list;
struct addrinfo *ai_ptr;
struct addrinfo ai_hints;
const char *node;
const char *service;
int new_s;
modbus_tcp_pi_t *ctx_tcp_pi;

if (ctx == NULL) {
    errno = EINVAL;
    return -1;
}

ctx_tcp_pi = ctx->backend_data;

// insert this

ifdef OS_WIN32

if (_modbus_tcp_init_win32() == -1) {
    return -1;
}

endif

// insert this

….
}

  1. The code in the test unit-test-client.c line 258

rc = modbus_read_registers(ctx, UT_REGISTERS_ADDRESS,
0, tab_rp_registers);
printf("3/5 modbus_read_registers (0): ");
if (rc != 0) {
printf("FAILED (nb points %d)\n", rc);
goto close;
}
printf("OK\n");
looks like incorrect because after the call wit nb=0 the rc is -1 as result of an error

if (nb < 1 || MODBUS_MAX_READ_REGISTERS < nb)
in modbus.c line 770
and an exception MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE from the server. It has to be changed to:

if (rc != -1) {
printf("FAILED (nb points %d)\n", rc);
goto close;
}
printf("OK\n");
Probably it is not Win32 only.

  1. The Win32 implementation of modbus is a DLL. This means that the test program or any other programs have separate errno from the DLL and communication about errors datails between EXE and DLL is not possible: EXE reads error value from anther slot than DLL uses for writing. Only in case of using the modbus as a static LIB the test passes. Any idea how to use modbus as DLL and accessible errno?

Best regards, vgrin!

@vgrin

This comment has been minimized.

Show comment
Hide comment
@vgrin

vgrin Jan 8, 2014

The same problem with nb=0 takes place in random-test-client.c

ERROR Illegal data value
ERROR modbus_read_bits
Address = 99, nb = 0
...
ERROR Illegal data value
ERROR modbus_read_registers (-1)
Address = 99, nb = 0
...
ERROR Illegal data value
ERROR modbus_read_and_write_registers (-1)
Address = 99, nb = 0
Test: 3 FAILS

Either
for (addr = ADDRESS_START; addr <= ADDRESS_END; addr++)

should be changed to

for (addr = ADDRESS_START; addr < ADDRESS_END; addr++)

or any read operations should be skipped for nb == 0?
The write operations with zero size are allowed, except _FC_WRITE_AND_READ_REGISTERS. Is it by design?

vgrin commented Jan 8, 2014

The same problem with nb=0 takes place in random-test-client.c

ERROR Illegal data value
ERROR modbus_read_bits
Address = 99, nb = 0
...
ERROR Illegal data value
ERROR modbus_read_registers (-1)
Address = 99, nb = 0
...
ERROR Illegal data value
ERROR modbus_read_and_write_registers (-1)
Address = 99, nb = 0
Test: 3 FAILS

Either
for (addr = ADDRESS_START; addr <= ADDRESS_END; addr++)

should be changed to

for (addr = ADDRESS_START; addr < ADDRESS_END; addr++)

or any read operations should be skipped for nb == 0?
The write operations with zero size are allowed, except _FC_WRITE_AND_READ_REGISTERS. Is it by design?

@vgrin

This comment has been minimized.

Show comment
Hide comment
@vgrin

vgrin Jan 9, 2014

A small fix of the bandwidth-client.c for Win32:

/*
...
*/

#include <stdio.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
#include <time.h>
#if !defined(_MSC_VER)
#include <sys/time.h>
#endif
#include <errno.h>

#include <modbus.h>

#define G_MSEC_PER_SEC 1000

uint32_t gettime_ms(void)
{
#if defined(_MSC_VER)
return GetTickCount();
#else
struct timeval tv;
gettimeofday (&tv, NULL);

return (uint32_t) tv.tv_sec * 1000 + tv.tv_usec / 1000;

#endif
}

....

vgrin commented Jan 9, 2014

A small fix of the bandwidth-client.c for Win32:

/*
...
*/

#include <stdio.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
#include <time.h>
#if !defined(_MSC_VER)
#include <sys/time.h>
#endif
#include <errno.h>

#include <modbus.h>

#define G_MSEC_PER_SEC 1000

uint32_t gettime_ms(void)
{
#if defined(_MSC_VER)
return GetTickCount();
#else
struct timeval tv;
gettimeofday (&tv, NULL);

return (uint32_t) tv.tv_sec * 1000 + tv.tv_usec / 1000;

#endif
}

....

@vgrin

This comment has been minimized.

Show comment
Hide comment
@vgrin

vgrin Jan 9, 2014

A typing mistake in the bandwidth-client.c line 180:

nb_points = MODBUS_MAX_RW_WRITE_REGISTERS;

to

nb_points = MODBUS_MAX_WR_WRITE_REGISTERS;

vgrin commented Jan 9, 2014

A typing mistake in the bandwidth-client.c line 180:

nb_points = MODBUS_MAX_RW_WRITE_REGISTERS;

to

nb_points = MODBUS_MAX_WR_WRITE_REGISTERS;

@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane Jan 9, 2014

Owner

@vgrin did look to other github issues about Win32 before posting?
Could you create a separate issue for each point (starting from now so don't report again in seperate issues the current comments)?

Owner

stephane commented Jan 9, 2014

@vgrin did look to other github issues about Win32 before posting?
Could you create a separate issue for each point (starting from now so don't report again in seperate issues the current comments)?

@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane Jan 9, 2014

Owner

I very difficult to answer clearly to your comments (one issue must be related to only one subject).

1 - I've added the change in modbus_tcp_pi_listen in a commit in my PC but before I push it, could you confirm you've tested it?
2 - the tests have been rewritten a month ago so could you report again against the current master branch of libmodbus?
3 - In "small fix of the bandwidth-client.c for Win32", the code is not properly formatted for github, please use github flavored markdown documentation for your next postings, it seems to be an improvement not a fix, right?
4 - MODBUS_MAX_RW_WRITE_REGISTERS not in master anymore

Thank you for your reports.

Owner

stephane commented Jan 9, 2014

I very difficult to answer clearly to your comments (one issue must be related to only one subject).

1 - I've added the change in modbus_tcp_pi_listen in a commit in my PC but before I push it, could you confirm you've tested it?
2 - the tests have been rewritten a month ago so could you report again against the current master branch of libmodbus?
3 - In "small fix of the bandwidth-client.c for Win32", the code is not properly formatted for github, please use github flavored markdown documentation for your next postings, it seems to be an improvement not a fix, right?
4 - MODBUS_MAX_RW_WRITE_REGISTERS not in master anymore

Thank you for your reports.

@stephane stephane closed this Jan 9, 2014

@vgrin

This comment has been minimized.

Show comment
Hide comment
@vgrin

vgrin Jan 9, 2014

Hello Stephane,
I did look, but I have not found these issues. Sorry, if I have duplicated something. I am new in github. Thank you for understanding.

  1. About modbus_tcp_pi_listen – of course I have tested and it works. BTWL any other reported issues have been fixed in my PC and they are working as well.
  2. Sorry, I didn’t know. Where should I look for current master branch ?
  3. It is not an improvement – there no neither <sys/time.h> nor gettimeofday under Win32

vgrin commented Jan 9, 2014

Hello Stephane,
I did look, but I have not found these issues. Sorry, if I have duplicated something. I am new in github. Thank you for understanding.

  1. About modbus_tcp_pi_listen – of course I have tested and it works. BTWL any other reported issues have been fixed in my PC and they are working as well.
  2. Sorry, I didn’t know. Where should I look for current master branch ?
  3. It is not an improvement – there no neither <sys/time.h> nor gettimeofday under Win32

stephane added a commit that referenced this issue Jan 9, 2014

@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane Jan 9, 2014

Owner
  1. OK pushed
  2. git clone https://github.com/stephane/libmodbus.git or download zip of master
  3. the current master code don't use gettimeofday for win32 but
SYSTEMTIME st;
GetLocalTime(&st);
tv.tv_sec = st.wSecond;
tv.tv_usec = st.wMilliseconds * 1000;
```, so I still think it's an improvement not a fix. Code pushed too, could you test benchmark client of master branch?
Owner

stephane commented Jan 9, 2014

  1. OK pushed
  2. git clone https://github.com/stephane/libmodbus.git or download zip of master
  3. the current master code don't use gettimeofday for win32 but
SYSTEMTIME st;
GetLocalTime(&st);
tv.tv_sec = st.wSecond;
tv.tv_usec = st.wMilliseconds * 1000;
```, so I still think it's an improvement not a fix. Code pushed too, could you test benchmark client of master branch?
@vgrin

This comment has been minimized.

Show comment
Hide comment
@vgrin

vgrin Jan 10, 2014

I have sent you e-mail with changes.
Have a nice weekend.

vgrin commented Jan 10, 2014

I have sent you e-mail with changes.
Have a nice weekend.

@vgrin

This comment has been minimized.

Show comment
Hide comment
@vgrin

vgrin Jan 16, 2014

About ever passed “Timeout of 3ms between bytes“ test – please change 500 microseconds to 5 milliseconds
File “tests\unit-test-server.c” line 204
from
usleep(500);
to
usleep(5000);

vgrin commented Jan 16, 2014

About ever passed “Timeout of 3ms between bytes“ test – please change 500 microseconds to 5 milliseconds
File “tests\unit-test-server.c” line 204
from
usleep(500);
to
usleep(5000);

@PointOnePA

This comment has been minimized.

Show comment
Hide comment
@PointOnePA

PointOnePA Jan 19, 2014

Sorry, I'm new to GIT Hub and using it for the first time, so I don't know it if is okay to post a comment on a closed issue.

I'm playing with libmodbus for Win32 (Visual Studio 2010) and I'll help on the unit tests once I get it running, but I thought I would mention a TYPO in modbus-tcp.c. line 557

line 557 "if (modbustcp_init_win32() == -1) {"
but
line 67 defines static in t _modbus_tcp_init_win32(void)

Note the two missing underscores indicated with asterisks: _modbus_tcp_init_win32

Once I added the two underscores on line 557:
if (_modbus_tcp_init_win32() == -1) {
the library compiles successfully in Visual Studio 2010, creating modbus.dll

FYI

PointOnePA commented Jan 19, 2014

Sorry, I'm new to GIT Hub and using it for the first time, so I don't know it if is okay to post a comment on a closed issue.

I'm playing with libmodbus for Win32 (Visual Studio 2010) and I'll help on the unit tests once I get it running, but I thought I would mention a TYPO in modbus-tcp.c. line 557

line 557 "if (modbustcp_init_win32() == -1) {"
but
line 67 defines static in t _modbus_tcp_init_win32(void)

Note the two missing underscores indicated with asterisks: _modbus_tcp_init_win32

Once I added the two underscores on line 557:
if (_modbus_tcp_init_win32() == -1) {
the library compiles successfully in Visual Studio 2010, creating modbus.dll

FYI

@vgrin

This comment has been minimized.

Show comment
Hide comment
@vgrin

vgrin Jan 21, 2014

@PointOnePA
yes, I have sent this fix to Stephane as well
With all my changes all test are running.

vgrin commented Jan 21, 2014

@PointOnePA
yes, I have sent this fix to Stephane as well
With all my changes all test are running.

mk8 added a commit to mk8/libmodbus that referenced this issue Jan 29, 2014

mk8 added a commit to mk8/libmodbus that referenced this issue Jan 29, 2014

mk8 added a commit to mk8/libmodbus that referenced this issue Jan 29, 2014

@conect

This comment has been minimized.

Show comment
Hide comment
@conect

conect Jan 27, 2015

I'm using linux and just cloned the current master but neverthelesse I get:
ERROR Illegal data value
ERROR modbus_read_and_write_registers (-1)
Address = 99, nb = 0
Test: 3 FAILS

on executing ./random-test-client

conect commented Jan 27, 2015

I'm using linux and just cloned the current master but neverthelesse I get:
ERROR Illegal data value
ERROR modbus_read_and_write_registers (-1)
Address = 99, nb = 0
Test: 3 FAILS

on executing ./random-test-client

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment