-
Notifications
You must be signed in to change notification settings - Fork 6
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
chan_simpleusb, chan_usbradio: Add parameter to make scaling/clipping optional, fix issue #399 #418
base: master
Are you sure you want to change the base?
Conversation
…ew rxaudiostats parameter to simpleusb.conf, which enables a new function check_rx_audio() in chan_simpleusb.c to be called during processing of received USB audio frames. If ADC clipping is then detected GPIO4 is set for 500mS to support illumination of a node / audio interface Clip LED. Statistics collected also include peak and average RMS audio levels averaged over the previous 1 second, which can be displayed from the simpleusb-tune-menu 'R' option or AMI 'susb tune menu-support a' function.
…' prefix to function names. Change rxaudiostats conf parameter name to 'checkrxaudio' and type to int, with value indicating GPIO# to use (or 0 to disable feature), validate value in hidhdwconfig()
…. Update susb tune-menu options to be consistent with that of usbradio. Default checkrxaudio conf param to off for usbradio. Add 'TBR' comments on some old sections of code that are present in both chan_simpleusb.c and chan_usbradio.c that will cause digital clipping of tx audio.
…O pin to use for the ADC Clip Detect Feature (0=disabled)). Set default value of clipledgpio to disabled in chan_[simpleusb|usbradio].c (so writing to a GPIO won't be automatically enabled during ASL updates), but enabled in configs/rpt/simpleusb.conf.
…raw Tx audio by 1.1 and (usbradio only) scales raw Rx audio by 0.8. (This code should marked TBR should be removed asap as described in issue AllStarLink#399.) Move call to check_rx_audio() to above where the 0.8 rx audio scaling is done in chan_usbradio.c.
…stats() per review comment
…arify that it will print using ast_verbose() if passed in fd < 0
…scaling/clipping code to remain unchanged for existing installs or if user enables parameter in simpleusb/usbradio .conf.
(BTW if anyone knows why github is listing commits from last month that are unrelated to this PR or how I can prevent that I'd be interested to know. Github does show the correct file changes so that's not an issue and the PR should be good to go, but I have no idea why it's listing on this tab commits from previous PRs. I did of course sync my fork before any changes, and then created a new branch so it would not show any previous commits, and then did the PR from that branch.) |
Are you cloning from the latest version of master before making your change on your branch and making a PR? In your fork, you may have to pull from "upstream" first. Maybe sync your fork before making any changes to ensure you're starting fresh. If the merger makes sure to squash the changes I think it should be fine, but I'd really prefer there just be a single commit in PRs so we know everything's good to go. I can't even tell what the commit description would be the way it is now. Also, for the commit name, it should be something like:
or something like that, to conform with the Asterisk commit message format: https://docs.asterisk.org/Development/Policies-and-Procedures/Commit-Messages/ |
Yes, I cloned from latest master, and before that sync'd my fork on github. Maybe I need to somehow squash/rebase my fork/branch before starting on a new PR? But that would seem contradictory to the point of a RCS. Honestly, git is the most unintuitive software I have ever worked with in 30 years of being a software engineer. Would be great if Asterisk or ASL or anyone actually documented the full, complete and proper workflow (i.e. all git commands that need to be executed) to do a PR. Just the fact that github says here that I "made 18 commits last month" and lists only a bunch of old files, yet then on the Files Changed tab shows none of those files and there shows the correct changes, is quite contradictory and would appear to be a bug in github itself.
Just updated that. |
Did you also sync your local copy? I will frequently use :
|
My process to do the PR was:
From what you're suggesting, maybe the magic trick here is I need to add the |
It does take some time getting used to it, especially for less frequently performed operations. Here is the Asterisk wiki page on it: https://docs.asterisk.org/Development/Policies-and-Procedures/Code-Contribution/ Some of that is not applicable to You don't need the |
So, a rebase is normally required when a lot of time has passed since a change was written. I used to have to do this a lot with the Asterisk repo back when it was on Gerrit. If you make a change at a particular time, your commit is a diff against a particular commit. As time passes, other commits may get merged into the branch on which you originally made your commit, but your branch doesn't have those changes. A rebase effectively pulls in all those changes and "rebases" your commit on top of the current HEAD so those changes are in the branch, and your commit is against a newer version of the branch, effectively. Took me some time to internally intuit that but once you've seen it happen it makes more sense. I don't have to rebase often myself, usually only when there's some kind of conflict that necessitates one or when asked. |
Just pushed with updates for all above review comments. (I did also try Allan's suggestion of the git stash, git pull --rebase, git stash pop, but it seemed to have no effect, the PR still shows only a bunch of old files on this tab and only the correct new changes on the Files Changed tab.) |
Unfortunately the above wiki page seems very specific to Asterisk, uses yet another tool (gh) adding even more complexity, and like all these guides doesn't explain why each step is needed. It's just a long list of random commands and as soon as what you're doing in a specific context is a little bit different the whole guide becomes essentially useless. I've read books on git. I get the overall idea of what's it's doing. I even have a minor in math, and like graph theory, topology and that sort of stuff. But while git seems nice in theory the real-world implementation seems to be way more complicated than it should be. It occurred to me this is somewhat analogous to proper state machine design in embedded systems development. To create reliable, deterministic embedded software you often have to enumerate the possible states of a process and define all the actions that have to take place during all possible state transitions. The software configuration management process is very similar. There are states and transitions such as fork repo, sync fork, create branch, push changes, create PR, merge PR, that should be easy to enumerate. And for each such state transition there are very specific git commands that should be used, or weird things happen and various 'hacks' may then be required. Does anyone really fundamentally understand what all these commands are for the context of app_rpt and why they're needed, or do these bits and pieces just get hacked together pragmatically as needed? I see the above guide recommends doing:
after the git clone, and it'd be nice if they would explain why. In past development on other projects a git clone was always sufficient to get what you'd think would in fact be a clone of the master repo, and I've previously not needed to do additional checkouts, pulls and pushes before being able to do anything else. |
After more web searches I think I am starting to find some better guides on the PR process with git: |
Pushed requested whitespace cleanups. And also tried a few suggestions from the above 3 guides, but they all seemed to do absolutely nothing. I tried all the following, with the idea that maybe it would somehow sync up my directory to the main upstream repo and those old commits showing on this tab might go away, but as usual with git things don't seem to work very predictably. It never ceases to amaze how there are 1,000's of guides, posts, books, etc. on git, with 100's of different commands and 1,000's of differenct combinations/permutations, but they rarely seem to actually do anything. |
As I said, you don't need the
A I certainly hear what you're saying... it's taken me years of using git on a regular basis (weekly, if not daily) to feel fairly comfortable with it, but if I'm doing something rather out of the ordinary, then I still need to look up the right procedure. |
This link has a pretty good summary of the PR workflow, https://blog.scottlowe.org/2015/01/27/using-fork-branch-git-workflow and suggests:
Looks like I skipped (3) so maybe that's why old commits are listed on this page. |
Add legacyaudioscaling parameter to allow scaling/clipping code to remain unchanged for existing installs or if user enables parameter in simpleusb/usbradio .conf. Summary of changes:
Testing done:
Fixes issue #399.