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

null() Function Call? #14

Open
treybgreen opened this issue Jun 1, 2020 · 22 comments · Fixed by #16
Open

null() Function Call? #14

treybgreen opened this issue Jun 1, 2020 · 22 comments · Fixed by #16
Labels

Comments

@treybgreen
Copy link

treybgreen commented Jun 1, 2020

Python doesn't recognize the null() function call.

Line 129 of dis7.py is 1st example.

        for idx in range(0, self.recordLength):
            element = null()
            element.parse(inputStream)
            self.iffData.append(element)

Is this supposed to be None?

@DuffyScottC
Copy link

I've had this issue before multiple times. All instances of null should be changed to None to match python3

@treybgreen
Copy link
Author

I've had this issue before multiple times. All instances of null should be changed to None to match python3

Previous versions of python did not support null() either. Python2 has also reached end of life as of January 2020.

I have made this change in my own code. I do not support these messages in my application currently nor have a need for them, so I have not tested the results thoroughly.

I am willing to submit a pull request but I believe this may be an issue with the tool used to generate this code (I believe similar to the unneeded semi colons but have not looked for the cause of either). I do not have time to fix all of these issues right now, but may be able to contribute a little bit if needed.

@leif81
Copy link
Member

leif81 commented Jun 3, 2020

A while back I fixed a number of occurences of null() in the code base that were causing issues.

99b7938
10f241e

It looks like this one was missed.

@leif81 leif81 added the bug label Jun 3, 2020
@treybgreen
Copy link
Author

@leif81 Shall I submit a pull request?

@leif81
Copy link
Member

leif81 commented Jun 3, 2020

Sure though I'm not sure None is the right replacement. Comparing this with the fix to the other null() fixes it looks like element needs to be initialized to a the expected object type so that the right parse() method is called.

@leif81
Copy link
Member

leif81 commented Jun 3, 2020

If the java implementation is right then iffData is an array of bytes.

https://github.com/open-dis/open-dis-java/blob/master/src/main/java/edu/nps/moves/dis7/IFFData.java#L127

@leif81
Copy link
Member

leif81 commented Jun 3, 2020

My hunch is it may need to be changed to this

for idx in range(0, self.recordLength):
    val = inputStream.read_byte()
    self.iffData[  idx  ] = val

@leif81 leif81 mentioned this issue Jun 3, 2020
@treybgreen
Copy link
Author

Sure though I'm not sure None is the right replacement. Comparing this with the fix to the other null() fixes it looks like element needs to be initialized to a the expected object type so that the right parse() method is called.

I agree with not changing them to None.

My hunch is it may need to be changed to this

for idx in range(0, self.recordLength):
    val = inputStream.read_byte()
    self.iffData[  idx  ] = val

Based on the 2 previous commits referenced (99b7938, 10f241e), I believe we should be doing something more like this:

        for idx in range(0, self.recordLength):
            element = IffData()
            element.parse(inputStream)
            self.iffData.append(element)

There are still 31 different instances of null() which will need different versions of this. Another null() Example

@treybgreen
Copy link
Author

Edited Original post to reference

element = null()

@leif81
Copy link
Member

leif81 commented Jun 3, 2020

Any of us happen to have a sample IFF Data PDU to use as reference to confirm ?

@pulsebreaker
Copy link

So I stumbled upon this bug with the Electronic Emissions PDU. Does anyone have an indication when it will be solved? I understand everybody is busy but I need this fixed for specifically the EM PDU before march next year and I'm not sure I can do that myself.

@leif81
Copy link
Member

leif81 commented Dec 6, 2020

Hi @pulsebreaker thanks for the comment. Originally there were many instances of nulls in the code. There were more than I had time to fix all at once. So I've been fixing them in batches based on demand for particular pdus.

Electronic emission pdu is probably one I didn't do yet.

Could you share a sample electronic emission pdu packet from Wireshark? That would be very helpful to know if the change I make will work or not.

@pulsebreaker
Copy link

Thanks for the quick response!

Here is a single EM system PDU. I might also have multiple EM systems PDU's if that's easier but I have to dig a little.

EM_PDU_single_system.zip

leif81 added a commit that referenced this issue Dec 19, 2020
@leif81
Copy link
Member

leif81 commented Dec 19, 2020

@pulsebreaker thank-you for the sample. This was very helpful. I made a large change to the Electromagnetic Emission Pdu parser/serializer and used your sample packet in a unit test. Can you give it a try the latest code on master branch and let me know if it fixed the issue?

@pulsebreaker
Copy link

It certainly is a very big step forward for me, thanks a lot!

However I'm receiving a new error:

aPdu = createPdu(data)
File "C:\Users...\AppData\Local\Programs\Python\Python37-32\lib\site-packages\opendis\PduFactory.py", line 92, in createPdu
return getPdu(inputStream)
File "C:\Users\s...\AppData\Local\Programs\Python\Python37-32\lib\site-packages\opendis\PduFactory.py", line 75, in getPdu
pdu.parse(inputStream)
File "C:\Users...\AppData\Local\Programs\Python\Python37-32\lib\site-packages\opendis\dis7.py", line 5382, in parse
element.parse(inputStream)
File "C:\Users...\AppData\Local\Programs\Python\Python37-32\lib\site-packages\opendis\dis7.py", line 5441, in parse
self.trackJamRecord.parse(inputStream);
File "C:\Users...\AppData\Local\Programs\Python\Python37-32\lib\site-packages\opendis\dis7.py", line 2136, in parse
self.entityID.parse(inputStream)
File "C:\Users...\AppData\Local\Programs\Python\Python37-32\lib\site-packages\opendis\dis7.py", line 3302, in parse
self.siteID = inputStream.read_unsigned_short();
File "C:\Users...\AppData\Local\Programs\Python\Python37-32\lib\site-packages\opendis\DataInputStream.py", line 35, in read_unsigned_short
return struct.unpack('>H', self.stream.read(2))[0]
struct.error: unpack requires a buffer of 2 bytes

It looks like it has to do with self.trackJamRecord.parse(inputStream).

Apparently it is possible for this PDU to have no targets in the Track or Jam field. If that's the case than the value self.numberOfTargetsInTrackJam = 0 and self.trackJamRecord.parse(inputStream) causes the struct unpack error.

I made a very cheap and dirty little change in dis7.py just to check my theory:

5441 if self.numberOfTargetsInTrackJam > 0:
5442 self.trackJamRecord.parse(inputStream);

With this ugly little addition the error is gone and on first sight it looks like it parses everything correctly.

Hope this helps.

Again, thanks a lot for all the work! Looks like I can proceed and finish my project in time after all.

@leif81
Copy link
Member

leif81 commented Dec 20, 2020

@pulsebreaker Ah Ok, I understand it now. Try it now I pushed a fix.

leif81 added a commit that referenced this issue Dec 20, 2020
leif81 added a commit that referenced this issue Dec 20, 2020
@pulsebreaker
Copy link

Wonderful. Works like a charm.

Thanks a lot for the effort.

@leif81
Copy link
Member

leif81 commented Dec 20, 2020

Great news @pulsebreaker

Unrelated....Do you have other sample dis 7 packets you could share? I would add them to make our unit test coverage better.Any and all pdu types would be useful. But in particular entity state, fire, detonate, signal, transmitter, receiver would be really useful.

@pulsebreaker
Copy link

This is a pcap with the emission pdu's and a couple of entity state pdu's. Unfortunately that's all I have right now that is releasable to the internet. It's made by an outside company.

pdus.zip

I have to figure out a way to create pdu's which I can share. Let met think about that.

@leif81 leif81 linked a pull request Jan 13, 2021 that will close this issue
@ztolley
Copy link

ztolley commented Jan 10, 2023

I tried checking the code out and looking at dis7.py and it still has a bunch of null() calls, is this meant to be working?

@leif81
Copy link
Member

leif81 commented Jan 11, 2023

@ztolley Yes there are still many null's in the code base left over from the auto generated code tool. Over time I've been fixing the most obvious ones. The most commonly used pdu's have been fixed and don't have nulls anymore (e.g. Entity State, Signal, Transmitter, Fire, Detonation, Electronic Emission, etc). If there's a specific pdu you're trying to use that has a null in it let me know. And better yet, you're welcome to submit a PR for a particular fix to get things going in a hurry.

@ngjunsiang
Copy link
Contributor

ngjunsiang commented May 19, 2024

As of time of this comment the following classes are affected:

  • DirectedEnergyAreaAimpoint
  • GridAxisDescriptorVariable
  • SilentEntitySystem
  • StandardVariableSpecification
  • RecordSpecification
  • IntercomSignalPdu
  • ResupplyReceivedPdu
  • DirectedEnergyFirePdu
  • RecordQueryReliablePdu
  • EntityDamageStatusPdu
  • UaPdu
  • SeesPdu
  • MinefieldResponseNackPdu

After completing the type annotation, it's clear that these null() calls are supposed to be replaced by record instantiation calls, some of which don't exist yet because of the difficulty of reading bitfields with non-octet field sizes.

I am currently working on a PR for bitfields, after which I should be able to chip away at some of these.

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

Successfully merging a pull request may close this issue.

6 participants