search

RevolutionPi / piControl

Kernel module for data exchange with RevPi I/O-Modules and Gateways
81 stars 24 forks source link

Revpi 2647/use serdev highlevel functions aio #78

Closed linosanfilippo-kunbus closed 1 year ago

l1k commented 1 year ago

About commit f1ac37cb7e1c ("piControl: use pibridge_req_io() to send AIO config messages"):

Functionally, the commit is fine. I still have a recommendation: I would amend the definitions of SAioConfig and SAioInConfig in IoProtocol.h to drop the uHeader and i8uCrc members.

Next, drop all the code in piAIOComm_Config() to populate the uHeader fields and calculate the i8uCrc. All of that is just dead code because you let pibridge_req_io() construct the header and crc. It's confusing to retain that code even though it no longer has any use.

These changes will allow you to define AIO_CONFIG_DATA1/2/3_LEN as sizeof(SAioConfig) or sizeof(SAioInConfig), respectively, leading to (IMO) clearer code. Alternatively, you could forgo those macro definitions entirely and just use sizeof() directly in the pibridge_req_io() calls. If you feel that it adds value to document those packet sizes (it probably does), add the information to IoProtocol.h.

l1k commented 1 year ago

Ugh, I've realized only now that the aioConfig_s[i].uHeader.sHeaderTyp1.bitAddress field is necessary to find the SAioConfig for a given address. Maybe use a separate u8[] array to store the address for a given array index?