Skip to content
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

support for raspyrfm hardware with unittest #434

Open
wants to merge 1 commit into
base: rewrite
Choose a base branch
from

Conversation

Phunkafizer
Copy link

No description provided.

tests/hardware_raspyrfm.c Outdated Show resolved Hide resolved
tests/hardware_raspyrfm.c Outdated Show resolved Hide resolved
@Phunkafizer
Copy link
Author

While doing some tests concerning the topic https://forum.pilight.org/showthread.php?tid=3522 I got an issue; the hwtype was set too late so that pilight can handle multiple hardware. I forced push and will bring it also to the staging branch.

@CurlyMoo
Copy link
Contributor

I forced push and will bring it also to the staging branch.

What actually changed?

@Phunkafizer
Copy link
Author

I forced push and will bring it also to the staging branch.

What actually changed?

Also here, the setting of hwtype

@CurlyMoo
Copy link
Contributor

CurlyMoo commented Mar 3, 2019

I'm currently porting the hardware module to lua, but i noticed that the unittest doesn't yet cover everything.

What i countered till this far:

  • It doesn't test if all necessary rfmcfg values are set to the correct values
  • It doesn't test if the SPI device is opened with the correct speed.

@CurlyMoo
Copy link
Contributor

CurlyMoo commented Mar 3, 2019

What i also don't get is how you set a rawlen of 32 pulses on line 202, while you expect a rawlen of 50 pulses on line 114.


Ok, the send unittest now works in lua. Next step is the receiving end.

@Phunkafizer
Copy link
Author

What i also don't get is how you set a rawlen of 32 pulses on line 202, while you expect a rawlen of 50 pulses on line 114.

This is because the rfm69 module transforms pulselengths to a contineous bitstream. For every 31 µS a bit is generated. A value of 248 in txtestpulses generates 248/31 = 8 bits. The last value in txtestpulses (5952) results in 5952/31 = 192 bits to be generated, in this case represented by the last 24 bytes in txtestraw array.

@Phunkafizer
Copy link
Author

I'm currently porting the hardware module to lua, but i noticed that the unittest doesn't yet cover everything.

What i countered till this far:

* It doesn't test if all necessary `rfmcfg` values are set to the correct values

* It doesn't test if the SPI device is opened with the correct speed.

How can I test for correct speed? I think if the SPI can not be opened with the requested speed it should return an error.

Which test is missing? The frequency is checked in setFrequency, if SPI channel is not provided, raspyrfmHwInit would return an error.

@CurlyMoo
Copy link
Contributor

CurlyMoo commented Mar 4, 2019

Which test is missing?

Here various parameters are set, but the test doesn't check if they are all set with the correct values:
https://github.com/pilight/pilight/pull/434/files#diff-37f44c5e034922929cf9d82ace01b336R323

The frequency is checked in setFrequency

This one is set.

How can I test for correct speed? I think if the SPI can not be opened with the requested speed it should return an error.

This is what i did in the new serial unittest:

int tcgetattr(int fildes, struct termios *termios_p) {
	memset(termios_p, 0, sizeof(struct termios));
	return 0;
}

int tcsetattr(int fildes, int optional_actions, const struct termios *termios_p) {
	char buffer[1024];
	int l = 0;

	l += snprintf(buffer, 1024,
		"%4u",
		optional_actions);
	l += snprintf(&buffer[l], 1024-l,
		"%8u",
		termios_p->c_iflag);
	l += snprintf(&buffer[l], 1024-l,
		"%8u",
		termios_p->c_oflag);
	l += snprintf(&buffer[l], 1024-l,
		"%8u",
		termios_p->c_cflag);
	l += snprintf(&buffer[l], 1024-l,
		"%8u",
		termios_p->c_lflag);

	int i = 0;
	for(i=0;i<NCCS;i++) {
		l += snprintf(&buffer[l], 1024-l,
			"%4u",
			termios_p->c_cc[i]);
	}

	l += snprintf(&buffer[l], 1024-l,
		"%8u",
		termios_p->c_ispeed);
	l += snprintf(&buffer[l], 1024-l,
		"%8u",
		termios_p->c_ospeed);
	write(fildes, buffer, l);
	return 0;
}

And the other side:

static void callback(uv_fs_t *req) {
	unsigned int len = req->result;

	uv_fs_req_cleanup(req);
	FREE(req);

	if(len > 0) {
		if(strcmp(rbuffer, "   2       0       0    6321       0   0   0   0   0   0 150   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0    4097    4097") == 0) {
			pthread_attr_t attrs;
			pthread_attr_init(&attrs);
			pthread_attr_setdetachstate(&attrs, PTHREAD_CREATE_DETACHED);

			pthread_create(&pthread, NULL, thread, &fd);
			return;
		}
	}

	if(run == 1) {
		uv_fs_t *read_req = NULL;
		if((read_req = MALLOC(sizeof(uv_fs_t))) == NULL) {
			OUT_OF_MEMORY /*LCOV_EXCL_LINE*/
		}

		memset(rbuffer, '\0', BUFFER_SIZE);

		uv_buf_t buf = uv_buf_init(rbuffer, BUFFER_SIZE);
		uv_fs_read(uv_default_loop(), read_req, fd, &buf, 1, -1, callback);
	}
}

So, you could change the wiringXSPISetup to write the SPI parameters to the /dev/spidev0.0 file as just file content and then you check the parameters written to that file in a file read callback at the unittest side. That's at least what i did.

@Phunkafizer
Copy link
Author

Phunkafizer commented Mar 4, 2019

Here various parameters are set, but the test doesn't check if they are all set with the correct values:
https://github.com/pilight/pilight/pull/434/files#diff-37f44c5e034922929cf9d82ace01b336R323

The rfmcfg are hardcoded values for the RFM69 IC. Is it really needed to check for it? When the SPI is working, which is checked implicitely with RX/TX tests, it can not fail.

@CurlyMoo
Copy link
Contributor

CurlyMoo commented Mar 4, 2019

The rfmcfg are hardcoded values for the RFM69 IC. Is it really needed to check for it? When the SPI is working, which is checked implicitely with RX/TX tests, it can not fail.

How can i be sure about that when porint the whole thing to lua and not having the device itself.

@Phunkafizer
Copy link
Author

How can i be sure about that when porint the whole thing to lua and not having the device itself.

Good point. I added checking of rfmcfg.

@CurlyMoo
Copy link
Contributor

CurlyMoo commented Mar 5, 2019

So what happens when i do the following:

for(i=0;i<27;i++) {
   writeReg(i+1, i+1);
}

instead of

for(i=0; i<sizeof(rfmcfg) / sizeof(rfmcfg[0]); i++)
    writeReg(rfmcfg[i].reg, rfmcfg[i].val);

Does the unittest work or fail and why?

@Phunkafizer
Copy link
Author

Does the unittest work or fail and why?

It will fail. You are writing wrong values in wrong registers. In the unittest all register writes are catched, and if a register/value pair there matches with rfmcfg, I set a flag in rfmcfgcheck. When everything is flagged in the rfmcfgcheck, the test will pass.

@CurlyMoo
Copy link
Contributor

CurlyMoo commented Mar 6, 2019 via email

@Phunkafizer
Copy link
Author

Those values are defined here:

const rfmreg_t rfmcfg[27] = {

@CurlyMoo
Copy link
Contributor

CurlyMoo commented Mar 6, 2019 via email

@Phunkafizer
Copy link
Author

That's what I said. At the hardware side not at the unit test side. Also, the new implementation will be in lua so the c defined struct of the module can't be used anymore .

Ok, this means the rfmcfg needs to be defined redundant also in the unittest. Then of course if a value in rfmcfg has to be changed it will also have to be changed in the unittest.

@CurlyMoo
Copy link
Contributor

CurlyMoo commented Mar 6, 2019

Ok, this means the rfmcfg needs to be defined redundant also in the unittest.

Welcome to the world of unittesting 😉

@CurlyMoo
Copy link
Contributor

I've ported the whole module to lua and all unittest succeed. You can check the work done in the raspyrfm repository. If the module doesn't work in real life, then the unittests are incomplete. I would suggest making sure the critical parts currently missing are covered in the final unittest.

I also believe nobody has ever got a more thorough review than you got. I see that as a compliment 😄

There are a few things i also want to see fixed before i finally merge:

  • The minrawlen and maxrawlen are set specificly to the values of the test. But in the final pilight implementation those are not the values used. mingaplen: 5100, maxgaplen: 99999, minrawlen: 25, maxrawlen: 512.
  • The same applies to the rx and tx tests. Those are not pulses i've ever seen in public. I would like to see actual pulses of a real protocol. You can use my 433nano and 433gpio as a reference. The pulses used there are those of a KlikAanKlikUit switch.
  • The parameter tests are also quite incomplete. Like missing values, wrong SPI channels, too many option set also those not defined etc.
  • In regard to the settings. Can we change the allowed frequency values to 433 and 868. I would also like to see the spi-channel renamed to channel.

Lastly:

  • Please make sure the style remains consistent to the pilight style.

@Phunkafizer
Copy link
Author

I'm back after a busy period.
I checked out the raspyrfm branch and was able to compile. Should the daemon run in this branch? Or is it only for the unittest? At the moment the daemon is looping @100% CPU in the loop in daemon.c line 1619

@CurlyMoo
Copy link
Contributor

CurlyMoo commented May 3, 2019

pilight programs are not supported in the rewrite branch. That's why i removed them in the latest commits.

@CurlyMoo
Copy link
Contributor

CurlyMoo commented May 3, 2019

Also, welcome back 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants