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.

Sign up for GitHub

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

[WIP] introduction of Arbitration_id class #296

Merged
merged 7 commits into from
Feb 26, 2019
Merged

[WIP] introduction of Arbitration_id class #296

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c909e98
introduce class for arbitration id (#144)
ebroecker Feb 23, 2019
84b10b3
make arbitration id default
ebroecker Feb 23, 2019
98daadf
make test more readable
ebroecker Feb 24, 2019
4bdb66f
fix test
ebroecker Feb 24, 2019
42ef05d
Merge branch 'development' into iss144
ebroecker Feb 24, 2019
1b22bda
rename class Arbitration_Id to ArbitrationId
ebroecker Feb 26, 2019
e68f2c0
Merge branch 'development' into iss144
ebroecker Feb 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/BusmasterRestbus.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,9 @@ def ticker_ecus(db, dbcname):
cycleTime = int(frame.attributes["GenMsgCycleTime"])
if float(cycleTime) > 0:
if cycleTime in bu._cycles:
bu._cycles[cycleTime].append(frame.id)
bu._cycles[cycleTime].append(frame.arbitration_id.id)
else:
bu._cycles[cycleTime] = [frame.id]
bu._cycles[cycleTime] = [frame.arbitration_id.id]
nodeList[bu.name] = bu.name + ".cpp"

timedPrototypes = ""
Expand Down
2 changes: 1 addition & 1 deletion examples/createCMacros.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ def main():
if cmdlineOptions.exportframe is not None:
for frameId in cmdlineOptions.exportframe.split(','):
try:
frame = db.frame_by_id(int(frameId))
frame = db.frame_by_id(canmatrix.Arbitration_Id(frameId))
except ValueError:
frame = db.frame_by_name(frameId)
if frame is not None:
Expand Down
2 changes: 1 addition & 1 deletion examples/createccl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def main():
txDict = {}
for frame in db.frames:
if ecu in frame.receivers:
receiveArray.append(frame.id)
receiveArray.append(frame.arbitration_id.id)
receiveDict[frame] = receiveIndex
receiveIndex += 1

Expand Down
65 changes: 33 additions & 32 deletions src/canmatrix/arxml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ def dump(dbs, f, **options):
pduTriggeringRefConditional, 'PDU-TRIGGERING-REF')
pduTriggeringRef.set('DEST', 'PDU-TRIGGERING')

if frame.extended == 0:
if frame.arbitration_id.extended == 0:
createSubElement(
canFrameTriggering,
'CAN-ADDRESSING-MODE',
Expand All @@ -211,7 +211,7 @@ def dump(dbs, f, **options):
canFrameTriggering,
'CAN-ADDRESSING-MODE',
'EXTENDED')
createSubElement(canFrameTriggering, 'IDENTIFIER', str(frame.id))
createSubElement(canFrameTriggering, 'IDENTIFIER', str(frame.arbitration_id.id))

pduTriggeringRef.text = "/Cluster/CAN/IPDUTRIGG_" + frame.name

Expand Down Expand Up @@ -1234,7 +1234,7 @@ def get_frame(frameTriggering, xmlRoot, multiplexTranslation, ns, float_factory)
if idele is None:
logger.info("found Frame %s without arbitration id %s", sn.text)
return None
idNum = int(idele.text)
arbitration_id = int(idele.text)

if None != frameR:
dlc = arGetChild(frameR, "FRAME-LENGTH", xmlRoot, ns)
Expand All @@ -1247,10 +1247,10 @@ def get_frame(frameTriggering, xmlRoot, multiplexTranslation, ns, float_factory)

pduFrameMapping[pdu] = arGetName(frameR, ns)

newFrame = canmatrix.Frame(arGetName(frameR, ns), id=idNum, size=int(dlc.text))
new_frame = canmatrix.Frame(arGetName(frameR, ns), size=int(dlc.text))
comment = get_desc(frameR, xmlRoot, ns)
if comment is not None:
newFrame.add_comment(comment)
new_frame.add_comment(comment)
else:
# without frameinfo take short-name of frametriggering and dlc = 8
logger.debug("Frame %s has no FRAME-REF" % (sn))
Expand All @@ -1260,7 +1260,7 @@ def get_frame(frameTriggering, xmlRoot, multiplexTranslation, ns, float_factory)
if pdu is None:
pdu = arGetChild(ipduTriggering, "I-SIGNAL-I-PDU", xmlRoot, ns) ## AR4.2
dlc = arGetChild(pdu, "LENGTH", xmlRoot, ns)
newFrame = canmatrix.Frame(sn.text,id=idNum,dlc=int(dlc.text) / 8)
new_frame = canmatrix.Frame(sn.text,id=arbitration_id,dlc=int(dlc.text) / 8)

if pdu is None:
logger.error("ERROR: pdu")
Expand All @@ -1279,13 +1279,13 @@ def get_frame(frameTriggering, xmlRoot, multiplexTranslation, ns, float_factory)
is_little_endian=is_little_endian,multiplex="Multiplexor")

multiplexor._initValue = 0
newFrame.add_signal(multiplexor)
new_frame.add_signal(multiplexor)
staticPart = arGetChild(pdu, "STATIC-PART", xmlRoot, ns)
ipdu = arGetChild(staticPart, "I-PDU", xmlRoot, ns)
if ipdu is not None:
pdusigmappings = arGetChild(ipdu, "SIGNAL-TO-PDU-MAPPINGS", xmlRoot, ns)
pdusigmapping = arGetChildren(pdusigmappings, "I-SIGNAL-TO-I-PDU-MAPPING", xmlRoot, ns)
get_signals(pdusigmapping, newFrame, xmlRoot, ns, None, float_factory)
get_signals(pdusigmapping, new_frame, xmlRoot, ns, None, float_factory)
multiplexTranslation[arGetName(ipdu, ns)] = arGetName(pdu, ns)

dynamicPart = arGetChild(pdu, "DYNAMIC-PART", xmlRoot, ns)
Expand All @@ -1303,14 +1303,15 @@ def get_frame(frameTriggering, xmlRoot, multiplexTranslation, ns, float_factory)
if ipdu is not None:
pdusigmappings = arGetChild(ipdu, "SIGNAL-TO-PDU-MAPPINGS", xmlRoot, ns)
pdusigmapping = arGetChildren(pdusigmappings, "I-SIGNAL-TO-I-PDU-MAPPING", xmlRoot, ns)
get_signals(pdusigmapping, newFrame, xmlRoot, ns, selectorId.text, float_factory)
get_signals(pdusigmapping, new_frame, xmlRoot, ns, selectorId.text, float_factory)

if newFrame.comment is None:
newFrame.add_comment(get_desc(pdu, xmlRoot, ns))
if new_frame.comment is None:
new_frame.add_comment(get_desc(pdu, xmlRoot, ns))

if extEle is not None:
if extEle.text == 'EXTENDED':
newFrame.extended = 1
if extEle is not None and extEle.text == 'EXTENDED':
new_frame.arbitration_id = canmatrix.Arbitration_Id(arbitration_id, extended = True)
else:
new_frame.arbitration_id = canmatrix.Arbitration_Id(arbitration_id, extended=False)

timingSpec = arGetChild(pdu, "I-PDU-TIMING-SPECIFICATION", xmlRoot, ns)
if timingSpec is None:
Expand All @@ -1327,48 +1328,48 @@ def get_frame(frameTriggering, xmlRoot, multiplexTranslation, ns, float_factory)
timePeriod = arGetChild(cyclicTiming, "TIME-PERIOD", xmlRoot, ns)

if cyclicTiming is not None and eventTiming is not None:
newFrame.add_attribute("GenMsgSendType", "cyclicAndSpontanX") # CycleAndSpontan
new_frame.add_attribute("GenMsgSendType", "cyclicAndSpontanX") # CycleAndSpontan
if minimumDelay is not None:
newFrame.add_attribute("GenMsgDelayTime", str(int(float_factory(minimumDelay.text) * 1000)))
new_frame.add_attribute("GenMsgDelayTime", str(int(float_factory(minimumDelay.text) * 1000)))
if repeats is not None:
newFrame.add_attribute("GenMsgNrOfRepetitions", repeats.text)
new_frame.add_attribute("GenMsgNrOfRepetitions", repeats.text)
elif cyclicTiming is not None:
newFrame.add_attribute("GenMsgSendType", "cyclicX") # CycleX
new_frame.add_attribute("GenMsgSendType", "cyclicX") # CycleX
if minimumDelay is not None:
newFrame.add_attribute("GenMsgDelayTime", str(int(float_factory(minimumDelay.text) * 1000)))
new_frame.add_attribute("GenMsgDelayTime", str(int(float_factory(minimumDelay.text) * 1000)))
if repeats is not None:
newFrame.add_attribute("GenMsgNrOfRepetitions", repeats.text)
new_frame.add_attribute("GenMsgNrOfRepetitions", repeats.text)
else:
newFrame.add_attribute("GenMsgSendType", "spontanX") # Spontan
new_frame.add_attribute("GenMsgSendType", "spontanX") # Spontan
if minimumDelay is not None:
newFrame.add_attribute("GenMsgDelayTime", str(int(float_factory(minimumDelay.text) * 1000)))
new_frame.add_attribute("GenMsgDelayTime", str(int(float_factory(minimumDelay.text) * 1000)))
if repeats is not None:
newFrame.add_attribute("GenMsgNrOfRepetitions", repeats.text)
new_frame.add_attribute("GenMsgNrOfRepetitions", repeats.text)

if startingTime is not None:
value = arGetChild(startingTime, "VALUE", xmlRoot, ns)
newFrame.add_attribute("GenMsgStartDelayTime", str(int(float_factory(value.text) * 1000)))
new_frame.add_attribute("GenMsgStartDelayTime", str(int(float_factory(value.text) * 1000)))
elif cyclicTiming is not None:
value = arGetChild(timeOffset, "VALUE", xmlRoot, ns)
if value is not None:
newFrame.add_attribute("GenMsgStartDelayTime", str(int(float_factory(value.text) * 1000)))
new_frame.add_attribute("GenMsgStartDelayTime", str(int(float_factory(value.text) * 1000)))

value = arGetChild(repeatingTime, "VALUE", xmlRoot, ns)
if value is not None:
newFrame.add_attribute("GenMsgCycleTime", str(int(float_factory(value.text) * 1000)))
new_frame.add_attribute("GenMsgCycleTime", str(int(float_factory(value.text) * 1000)))
elif cyclicTiming is not None:
value = arGetChild(timePeriod, "VALUE", xmlRoot, ns)
if value is not None:
newFrame.add_attribute("GenMsgCycleTime", str(int(float_factory(value.text) * 1000)))
new_frame.add_attribute("GenMsgCycleTime", str(int(float_factory(value.text) * 1000)))


# pdusigmappings = arGetChild(pdu, "SIGNAL-TO-PDU-MAPPINGS", arDict, ns)
# if pdusigmappings is None or pdusigmappings.__len__() == 0:
# logger.debug("DEBUG: Frame %s no SIGNAL-TO-PDU-MAPPINGS found" % (newFrame.name))
# logger.debug("DEBUG: Frame %s no SIGNAL-TO-PDU-MAPPINGS found" % (new_frame.name))
pdusigmapping = arGetChildren(pdu, "I-SIGNAL-TO-I-PDU-MAPPING", xmlRoot, ns)

if pdusigmapping is not None and pdusigmapping.__len__() > 0:
get_signals(pdusigmapping, newFrame, xmlRoot, ns, None, float_factory)
get_signals(pdusigmapping, new_frame, xmlRoot, ns, None, float_factory)

# Seen some pdusigmapping being [] and not None with some arxml 4.2
else: ##AR 4.2
Expand All @@ -1386,10 +1387,10 @@ def get_frame(frameTriggering, xmlRoot, multiplexTranslation, ns, float_factory)
if signaltopdumaps is None:
logger.debug("DEBUG: AR4.x PDU %s no SIGNAL-TO-PDU-MAPPINGS found - no signal extraction!" % (arGetName(ipdus, ns)))
# signaltopdumap = arGetChild(signaltopdumaps, "I-SIGNAL-TO-I-PDU-MAPPING", arDict, ns)
get_signals(signaltopdumaps, newFrame, xmlRoot, ns, None, float_factory)
get_signals(signaltopdumaps, new_frame, xmlRoot, ns, None, float_factory)
else:
logger.debug("DEBUG: Frame %s (assuming AR4.2) no PDU-TRIGGERINGS found" % (newFrame.name))
return newFrame
logger.debug("DEBUG: Frame %s (assuming AR4.2) no PDU-TRIGGERINGS found" % (new_frame.name))
return new_frame


def get_desc(element, arDict, ns):
Expand Down
85 changes: 56 additions & 29 deletions src/canmatrix/canmatrix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class EncodingComplexMultiplexed(ExceptionTemplate): pass
class MissingMuxSignal(ExceptionTemplate): pass
class DecodingComplexMultiplexed(ExceptionTemplate): pass
class DecodingFrameLength(ExceptionTemplate): pass
class ArbitrationIdOutOfRange(ExceptionTemplate): pass

@attr.s
class ecu(object):
Expand Down Expand Up @@ -551,6 +552,48 @@ def pack_bitstring(length, is_float, value, signed):

return bitstring

@attr.s(cmp=False)
class Arbitration_Id(object):
ebroecker marked this conversation as resolved.
Show resolved Hide resolved
standard_id_mask = ((1 << 11) - 1)
extended_id_mask = ((1 << 29) - 1)
compound_extended_mask = (1 << 31)

id = attr.ib(default=None)
extended = attr.ib(default=None)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
extended = attr.ib(default=None)
extended = attr.ib(default=True)


def __attrs_post_init__(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tending away from writing __attrs_post_init__s anymore. I think this could be just a helper @classmethod that the from_compound_integer() calls. We should be able to construct any class we want rather than having to create it and then modify to get to any arbitrary set of attributes.

if self.extended is None or self.extended:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if self.extended is None or self.extended:
if self.extended:

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was the requirement to get both arbitration ids - the extended and the basic one - with the "by_id" method. Therefore the "None" was. If you set extended to "None" and compare two arbitration ids you'll get a true for both - the extended and the basic version.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... maybe by_id() should be return itertools.chain.from_iterable(by_id(id, extended) for type in (False, True)) or less niftily return by_id(id, False) + by_id(id, True). (aside from the recursion in both cases...). I'm really not sure. I'm just taking to simpler classes at this point. :| Do what you want and we'll see how we feel about it later.

mask = self.extended_id_mask
else:
mask = self.standard_id_mask

if self.id != self.id & mask:
raise ArbitrationIdOutOfRange('ID out of range')

@classmethod
def from_compound_integer(cls, i):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def from_compound_integer(cls, i):
def from_compound_integer(cls, i, extended=True):

And associated changes inside the function.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... the compound_integer already has the extendedinformation in bit 31 - so no need for the extended attribute. Or do I miss something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:[ oh right, it's compound... I'm really not on my game today, sorry.

return cls(
id=i & cls.extended_id_mask,
extended=(i & cls.compound_extended_mask) != 0,
)

def to_compound_integer(self):
if self.extended:
return self.id | self.compound_extended_mask
else:
return self.id

def __eq__(self, other):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be able to drop this after the above changes? The default attrs-provided __eq__ should work and if it doesn't we should probably try to look for a way to make it.

return (
self.id == other.id
and (
self.extended is None
or other.extended is None
or self.extended == other.extended
)
)


@attr.s(cmp=False)
class Frame(object):
"""
Expand All @@ -574,10 +617,10 @@ class Frame(object):
"""

name = attr.ib(default="")
id = attr.ib(type=int, default=0)
arbitration_id = attr.ib(converter=Arbitration_Id.from_compound_integer, default=0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to be so verbose re: arbitration_id over id? It might be better, but I'm not sure there's likely much confusion with just id. Either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attention, id is a python keyword. It would lead to "shadowing" warning.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it presumably has been already. I can't say I like it but it only shadows in the class definition itself. Once you are outside the class definition there's no more shadowing since it's self.id. Since you mostly shouldn't use id() at all anyways... I think I'm ok with this. It's a hazard of mixing declarative with imperative like attrs does in this style usage.

size = attr.ib(default=0)
transmitters = attr.ib(type=list, factory=list)
extended = attr.ib(type=bool, default=False)
# extended = attr.ib(type=bool, default=False)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, So we're changing from default=False to default=True effectively above. I'm not sure which it should be, or how much it matters.

Do delete this before it gets left hanging around.

is_complex_multiplexed = attr.ib(type=bool, default=False)
is_fd = attr.ib(type=bool, default=False)
comment = attr.ib(default="")
Expand All @@ -604,7 +647,7 @@ def is_multiplexed(self):

@property
def pgn(self):
return CanId(self.id).pgn
return CanId(self.arbitration_id.id).pgn

@pgn.setter
def pgn(self, value):
Expand Down Expand Up @@ -635,8 +678,8 @@ def source(self, value):

def recalc_J1939_id(self):
"""Recompute J1939 ID"""
self.id = (self.j1939_source & 0xff) + ((self.j1939_pgn & 0xffff) << 8) + ((self.j1939_prio & 0x7) << 26)
self.extended = True
self.arbitration_id.id = (self.j1939_source & 0xff) + ((self.j1939_pgn & 0xffff) << 8) + ((self.j1939_prio & 0x7) << 26)
self.arbitration_id.extended = True
self.is_j1939 = True

# @property
Expand Down Expand Up @@ -1000,7 +1043,7 @@ def unpack(self, data, report_error=True):
rx_length = len(data)
if rx_length != self.size and report_error:
print(
'Received message 0x{self.id:08X} with length {rx_length}, expected {self.size}'.format(**locals()))
'Received message 0x{self.arbitration_id.id:08X} with length {rx_length}, expected {self.size}'.format(**locals()))
raise DecodingFrameLength
else:
little, big = self.bytes_to_bitstrings(data)
Expand Down Expand Up @@ -1316,32 +1359,16 @@ def delete_obsolete_defines(self):
for element in toBeDeleted:
del self.signal_defines[element]

def frame_by_id(self, Id, extended=None):
def frame_by_id(self, arbitration_id):
"""Get Frame by its arbitration id.

:param Id: Frame id as str or int
:param extended: is it an extended id? None means "doesn't matter"
:param Id: Frame id as canmatrix.Arbitration_Id
:rtype: Frame or None
"""
Id = int(Id)
extendedMarker = 0x80000000
for test in self.frames:
if test.id == Id:
if extended is None:
# found ID while ignoring extended or standard
return test
elif test.extended == extended:
# found ID while checking extended or standard
return test
else:
if extended is not None:
# what to do if Id is not equal and extended is also provided ???
pass
else:
if test.extended and Id & extendedMarker:
# check regarding common used extended Bit 31
if test.id == Id - extendedMarker:
return test
if test.arbitration_id == arbitration_id:
# found ID while ignoring extended or standard
return test
return None

def frame_by_name(self, name):
Expand Down Expand Up @@ -1669,10 +1696,10 @@ def merge(self, mergeArray):
"""
for dbTemp in mergeArray:
for frame in dbTemp.frames:
copyResult = canmatrix.copy.copy_frame(frame.id, dbTemp, self)
copyResult = canmatrix.copy.copy_frame(frame.arbitration_id, dbTemp, self)
if copyResult == False:
logger.error(
"ID Conflict, could not copy/merge frame " + frame.name + " %xh " % frame.id + self.frame_by_id(frame.id).name)
"ID Conflict, could not copy/merge frame " + frame.name + " %xh " % frame.arbitration_id.id + self.frame_by_id(frame.arbitration_id).name)
for envVar in dbTemp.env_vars:
if envVar not in self.env_vars:
self.add_env_var(envVar, dbTemp.envVars[envVar])
Expand Down
2 changes: 1 addition & 1 deletion src/canmatrix/cmcsv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ def dump(db, thefile, delimiter=',', **options):
if frame.is_complex_multiplexed:
logger.error("export complex multiplexers is not supported - ignoring frame " + frame.name)
continue
frameHash[int(frame.id)] = frame
frameHash[int(frame.arbitration_id.id)] = frame

# set row to first Frame (row = 0 is header)
row = 1
Expand Down
Loading