parameter rack and slot encoding

[Justin Mclean <ju...@classsoftware.com>]

Hi,

Was just writing a test for parameter enough and notice an issue (sonar cube thought something was up as well):

This line in IsoTPProtocol:
out.writeByte((byte) ((tsap.getRackNumber() << 4) | (tsap.getSlotNumber())));

I think should be:
out.writeByte((byte) ((tsap.getRackNumber() << 4) | (tsap.getSlotNumber() & 0x0F)));

Is it right to assume that rack and slot have a maximum value of 128 as both are packed into a single byte here. Is that always the case? I see that parseCallParameter for instance assumes this.

If so it's probably not a big deal as a slot number that is passed in is never going to be too large and overwrite the rack 1/2 of that byte but may be best to make the change just in case it ever does.

Perhaps we should make that more implicit in the interface and not use bytes but classes for rack and slot?

Thanks,
Justin

Re: parameter rack and slot encoding

[Justin Mclean <ju...@me.com>]

Hi,

> I was also thinking of changing the method signatures to int to avoid all the casting needed when using the API. When doing that I would have provided some Assertions that check the parameter values. 

May be a good idea to also do that with bytes (i.e. change to ints, mask and/or assert)  there’s a lot of casting going on in using some APIs as well.

> There seems to be one additional way to provide constraint information: Jetbrains Constraint annotations … even if they don’t seem to provide any logic I don’t know if using proprietary Annotations like those in an open-source project.

I think we should probably pass on that, making people use one IDE (even if it’s currently popular) or making one work better is going to reduce the pool of potential users.

Thanks,
Justin

Re: parameter rack and slot encoding

[Christofer Dutz <ch...@c-ware.de>]

Hi Justin,

I had been thinking about this problem too for quite some time, but I came to the conclusion to rather go the other way.
Initially for example I wanted to ensure no wrong types could be used to read/write items and provided a pile of classes to ensure nothing goes wrong. But this made the API and maintenance of the API quite heavy. So, I removed this, and decided to rather go down the path of throwing errors in case of wrong types.

I was also thinking of changing the method signatures to int to avoid all the casting needed when using the API. When doing that I would have provided some Assertions that check the parameter values. 

There seems to be one additional way to provide constraint information: Jetbrains Constraint annotations … even if they don’t seem to provide any logic I don’t know if using proprietary Annotations like those in an open-source project. I guess when using IntellIJ, this could provide neat help (and for me I can’t recall there is any IDE besides IntelliJ ;-)) … perhaps someone here knows of something similar to provide contract information to method calls.

Chris




Am 08.01.18, 01:11 schrieb "Justin Mclean" <ju...@classsoftware.com>:

    Hi,
    
    Was just writing a test for parameter enough and notice an issue (sonar cube thought something was up as well):
    
    This line in IsoTPProtocol:
    out.writeByte((byte) ((tsap.getRackNumber() << 4) | (tsap.getSlotNumber())));
    
    I think should be:
    out.writeByte((byte) ((tsap.getRackNumber() << 4) | (tsap.getSlotNumber() & 0x0F)));
    
    Is it right to assume that rack and slot have a maximum value of 128 as both are packed into a single byte here. Is that always the case? I see that parseCallParameter for instance assumes this.
    
    If so it's probably not a big deal as a slot number that is passed in is never going to be too large and overwrite the rack 1/2 of that byte but may be best to make the change just in case it ever does.
    
    Perhaps we should make that more implicit in the interface and not use bytes but classes for rack and slot?
    
    Thanks,
    Justin