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

better anonymous & tagged dongle handling, sdr file cleanup #279

Merged
merged 18 commits into from
Feb 26, 2016

Conversation

jpoirier
Copy link
Collaborator

This fixes/simplifies the handling of anonymous and tagged dongles.

@jpoirier jpoirier changed the title Sdr cleanup better anonymous & tagged dongle handling, sdr file cleanup Feb 22, 2016
@peepsnet
Copy link
Contributor

peepsnet commented Feb 22, 2016

There was talk on prioritizing SDR choices in the case of an SDR failure.

Assume 2 freq tuned antennas and dual SDR's
If an SDR fails in flight I believe the logical choice is to assign the working SDR to the 978 freq(if turned on in settings) so weather/nexrad is still available(1090 is shutdown and 978 assigned).

Does this code keep the stx:978:-24 and stx:1090:-28 or stx:0:21 per SDR PPM serial format??

The 0 in the serial simply means I do not care which freq you assign to it but I want the ppm setting

If my SDR's serials were set like this:
stx:978:-24 and stx:1090:-28
or
stx:978:-24 and stx:0:24

I want each SDR to be set to the freq in the serial or if a 0 then set other freq first then what is left is assigned to the 0 serial SDR

IF I have 978 turned on in the settings and no 978 serial is found then set the remaining/only SDR to 978 regardless of the serials

Does this make sense??

@jpoirier
Copy link
Collaborator Author

Does this code keep the stx:978:-24 and stx:1090:-28 or stx:0:21per SDR PPM serial format??

yes

stx:978:-24 and stx:1090:-28

these should both get identified and configured as tagged dongles

stx:978:-24 and stx:0:24

stx:978:-24 should get tagged and configured as a 978 dongle and stx:0:24 should get tagged as an anonymous dongle and configured to 1090

I want each SDR to be set to the freq in the serial or if a 0 then set other freq first then what is left is assigned to the 0 serial SDR

yes, this code should do that

IF I have 978 turned on in the settings and no 978 serial is found then set the remaining/only SDR to 978 regardless of the serials

this code won't do that; it prioritizes the serial id regardless of the dongle count. the main reason for writing an id string to a dongle is to mate the dongle with an antenna for a specific frequency.

edit: added reason for writing an id string to a dongle

@peepsnet
Copy link
Contributor

so If I have a stx:978:24 dongle and a stx:0:-21 dongle and the 978 dongle fails... that happens on reboot??

What if I have a stx:978:55 dongle and only the 1090 SDR turned on in the settings what happens on boot??

@jpoirier
Copy link
Collaborator Author

What if I have a stx:978:55 dongle and only the 1090 SDR turned on in the settings what happens on boot??

if only 1090 is turned on in the web ui settings and there's a single dongle with the id "stx:978:55" it won't be configured

so If I have a stx:978:24 dongle and a stx:0:-21 dongle and the 978 dongle fails... that happens on reboot??

assuming the stx:978:24 dongle is dead, ie it doesn't show up in the device count, the stx:0:-21 dongle will be configured to 978 provided 978 is enabled in the web ui settings and regardless of whether 1090 is enabled

@peepsnet
Copy link
Contributor

Great. Thats what I wanted to know!

}

// we only get here when it's an anonymous dongle
if uat_enabled && UATDev == nil {
Copy link

Choose a reason for hiding this comment

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

This still has the bug that I fixed in #278: if i 0 is anonymous and i 1 is wired to UAT, then only 0 will be activated on UAT.

The fix is to run the loop twice: first to assign non-anonymous dongles and the second to assign anonymous dongles. Ideally a third loop would assign all dongles, in case a user erroneously assigned both dongles to :1090. This logic would also implement #239.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, and thanks for the review! I'll fix and push it later today.

@jpoirier
Copy link
Collaborator Author

@yihchun it should be be working now but let me know if you see any (more) problems/flaws

log.Printf("rtl.GetDeviceUsbStrings id %d: %s\n", v, err)
}
}
}
Copy link

Choose a reason for hiding this comment

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

Looks great but I think your big comment block is incorrect; since loop 2 doesn't check, for example,
if uat_enabled && UATDev == nil && !rES.hasID(s)
loop 2 will automatically override (ignore?) any dual-assignments. In my opinion, ideal behavior would include such a check in loop 2 and then override that check in a loop 3, but the existing code is very close to ideal behavior. A case in which this might arise is if someone connected 4 SDRs: one 1090 connected to a ES antenna, two 978s connected to UAT antennas, and one anonymous connected to a generically-lengthed antenna, and the 1090 SDR fails; the existing code might assign the second 978 to ES when the correct choice may be the anonymous one.

But the code is already ideal for my setup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On Mon, Feb 22, 2016 at 11:39 AM, yihchun notifications@github.com wrote:

In main/sdr.go
#279 (comment):

}
  • // loop 2; assign anonymous dongles
  • for _, v := range unusedIDs {
  •   _, _, s, err := rtl.GetDeviceUsbStrings(v)
    
  •   if err == nil {
    
  •       if uat_enabled && UATDev == nil {
    
  •           createUATDev(v, s, false)
    
  •       } else if es_enabled && ESDev == nil {
    
  •           createESDev(v, s, false)
    
  •       }
    
  •   } else {
    
  •       log.Printf("rtl.GetDeviceUsbStrings id %d: %s\n", v, err)
    
  •   }
    
  • }
    +}

one caveat is that we only support up to two dongles, the first two seen by
librtlsdr (device id 0 and 1) :)

    // support two and only two dongles
    if count > 2 {
        count = 2
    }

Looks great but I think your big comment block is incorrect; since loop 2
doesn't check, for example,
if uat_enabled && UATDev == nil && !rES.hasID(s)
loop 2 will automatically override (ignore?) any dual-assignments. In my
opinion, ideal behavior would include such a check in loop 2 and then
override

I'm not sure I understand.

Once loop 1 finishes the only remaining enabled dongles must be
anonymous, therefore, in loop 2, rES.hasID(s) and rUAT.hasID(s) could never
be true for an enabled and as yet unassigned device.

unusedIDs contains no IDs prior to loop 1 and device ID 0 is only appended
to unusedIDs when said dongle has neither a 1090 or 978 serial, similar for
device ID 1. Therefore, when loop 2 starts we only range over device IDs
that weren't consumed in loop 1.

Copy link

Choose a reason for hiding this comment

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

one caveat is that we only support up to two dongles

OK, I forgot about that force count = 2. At this point I don't see any significant reason to do that force. That said, since the code does it, it's clear that the second loop is sufficient to fix any configurations such as ES/ES or UAT/UAT into better behavior.

Once loop 1 finishes the only remaining enabled dongles must be
anonymous

I'm not sure I see it that way. If both id 0 and id 1 have rES.hasID(s), then in loop 1, id 0 will be wired to ES, then the next iteration (still loop 1), id 1 will not satisfy ESDev == nil so it will fall through to the else, thus being added to the unusedIDs array. At this point you'll send that one to UAT, which is correct behavior but id 1 is certainly not anonymous (I'd call it "frequency-flexible," because some named SDRs can be frequency-flexible, such as stx:0:10).

(At this point we're discussing the correctness of the comment matrix.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On Mon, Feb 22, 2016 at 12:31 PM, yihchun notifications@github.com wrote:

In main/sdr.go
#279 (comment):

}
  • // loop 2; assign anonymous dongles
  • for _, v := range unusedIDs {
  •   _, _, s, err := rtl.GetDeviceUsbStrings(v)
    
  •   if err == nil {
    
  •       if uat_enabled && UATDev == nil {
    
  •           createUATDev(v, s, false)
    
  •       } else if es_enabled && ESDev == nil {
    
  •           createESDev(v, s, false)
    
  •       }
    
  •   } else {
    
  •       log.Printf("rtl.GetDeviceUsbStrings id %d: %s\n", v, err)
    
  •   }
    
  • }
    +}

one caveat is that we only support up to two dongles

OK, I forgot about that force count = 2. I thought you were going to fix
#239 #239 also. At this point I
don't see any significant reason to do that force.

Once loop 1 finishes the only remaining enabled dongles must be
anonymous

I'm not sure I see it that way. If both id 0 and id 1 have rES.hasID(s),
then in round 1, id 0 will be wired to ES, then the next iteration id 1
will not satisfy ESDev == nil so it will fall through to the else, thus
being added to the unusedIDs array. At this point you'll send that one to
UAT, which is correct behavior but id 1 is certainly not anonymous.

Lol. Okay, it only took you what 4 or 5 times to explain the dual
assignment scenario...thanks! I'll add the additional checks. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the term freq-flexible. I hope to contribute to the faq when this per SDR PPM is working.

@peepsnet
Copy link
Contributor

@jpoirier How can I pull these changes to my stratux. just your changed files and test

@jpoirier
Copy link
Collaborator Author

On Mon, Feb 22, 2016 at 12:55 PM, peepsnet notifications@github.com
wrote:

@jpoirier https://github.com/jpoirier How can I pull these changes to
my stratux. just your changed files and test

I think this should work

Assuming you have the latest stratux code, you can create and checkout a
new branch then: git pull origin pull/279/head

It'll merge everything then you can make, make install...

@peepsnet
Copy link
Contributor

Ok it appears merged based on the new log entries. but it assigned the SDRs backwards

wait... a second

@jpoirier
Copy link
Collaborator Author

On Mon, Feb 22, 2016 at 1:24 PM, peepsnet notifications@github.com wrote:

Ok it appears merged based on the new log entries. but it assigned the
SDRs backwards

The pull request (#280) is still open
so unless you explicitly pull and merge it yourself you won't have the
modified code.

What do you mean assigned backwards?

@peepsnet
Copy link
Contributor

It is working correctly... I didn't reboot the PI after changing the Serials. I just restarted the stratux service.

and your pull is #279

I booted with a stx:978:-24 dongle and a stx:0:-28 dongle. Pulled the 978 dongle and rebooted. I got the "freq-flex" dongle to be 978 with both 978 & 1090 on in settings.

Now we need more information on the Settings page to indicate the SDR might be turned on in settings but not responding. A user is able to turn on 2 SDRs with only 1(one) detected and the Status page indicated both SDR's are on.

Maybe even a warning indicating that 2 SDRs are enabled but only 1 detected.

Good job so far on this improvement.

@jpoirier
Copy link
Collaborator Author

On Mon, Feb 22, 2016 at 1:50 PM, peepsnet notifications@github.com wrote:

It is working correctly... I didn't reboot the PI after changing the
Serials. I just restarted the stratux service.

Awesome, and thanks for taking the time to test this out!

@yihchun
Copy link

yihchun commented Feb 23, 2016

Looks great!

cyoung added a commit that referenced this pull request Feb 26, 2016
better anonymous & tagged dongle handling, sdr file cleanup
@cyoung cyoung merged commit 002a99f into cyoung:master Feb 26, 2016
@jpoirier jpoirier deleted the sdr_cleanup branch February 27, 2016 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants