diff --git a/AGENTS.md b/AGENTS.md index 109fc39c6..3551917d2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -360,3 +360,35 @@ interface (that runs the real `usb_audio_disconnect`). A genuine bus drop looks different: `usbDetach` for the hub, serial and audio devices together and `serial.send: port not open!` — that one is electrical (RF resetting the hub), not software. + +**5. On at least one phone's USB-audio HAL, RX capture and TX playback +cannot share one USB rig at the same time.** Opening TX's `AudioTrack` while +`MicRecorder`'s RX `AudioRecord` is still open on the same USB device fails +at the ALSA proxy level, and every subsequent write is then silently +dropped — the generated waveform can be full-scale all the way to +`AudioTrack.write()` and still never reach the rig, with no exception and no +error surfaced to the app. Tell from log (vendor tags, MediaTek-specific +spelling, but the pattern generalizes): `AudioALSAPlaybackHandlerUsb: open(), +proxy_open fail, ret -12` right after `alsa_device_proxy: proxy_open() +pcm_is_ready() failed: cannot set hw params: I/O error`, then +`AudioALSAPlaybackHandlerUsb: write(), mPcm is NULL!` repeated once per +buffer for the rest of the transmission (thousands of lines on a 13 s over). +Correlates exactly with whether `AudioALSACaptureHandlerUsb` is open at that +moment — a reference app (QFT8) that drives the same radio to full output +runs strictly half-duplex: it closes its USB capture, plays, then reopens +capture, and its opens never fail. Fixed by doing the same thing around +every "over"/TUNE key-down: `MainViewModel`'s shared `beginKeying()`/ +`endKeying()` (already the single choke point for PTT and SCO around TX, see +items above) now calls `HamRecorder.pauseForTx()`/`resumeAfterTx()`, gated by +`MainViewModel.shouldPauseRxCaptureForTx()`: only for the QRP Labs QMX, and +only when RX is actually an Android capture session (not a LAN rig, not +already USB-direct) on the *same* USB VID:PID as TX's output (not USB-direct +output either, which is separately broken for unrelated reasons). Keep the +QMX gate: a matching VID:PID alone is true for every single-cable USB rig +(IC-7300, IC-705, IC-9700, FT-991A, Digirig, SignaLink...), and pausing RX +there blanks the waterfall during TX and breaks full-duplex satellite +monitoring for a HAL conflict that was only ever confirmed with a QMX. +Widen it only on a confirmed report from another rig. Pauses nest (a tune +carrier and an over can overlap), so `HamRecorder` counts them, and +`MicRecorder.stopRecord()` joins the old reader thread so a quick +resume can't leave two readers on one `AudioRecord`. diff --git a/ft8af/app/src/main/assets/rigaddress.txt b/ft8af/app/src/main/assets/rigaddress.txt index f0ec1d208..8b6ac2512 100644 --- a/ft8af/app/src/main/assets/rigaddress.txt +++ b/ft8af/app/src/main/assets/rigaddress.txt @@ -77,3 +77,4 @@ Yaesu FT-891 (Hamlib),40C,38400,26 ICOM IC-7300MK2,B6,115200,0 YAESU FTX-1(USB),00,38400,2 QRP Labs QMX,00,9600,27 +Qrp Labs QMX (Hamlib),809,9600,26 diff --git a/ft8af/app/src/main/cpp/ft8af_glue/hamlib_jni.cpp b/ft8af/app/src/main/cpp/ft8af_glue/hamlib_jni.cpp index 00843b216..e9a7a5782 100644 --- a/ft8af/app/src/main/cpp/ft8af_glue/hamlib_jni.cpp +++ b/ft8af/app/src/main/cpp/ft8af_glue/hamlib_jni.cpp @@ -64,6 +64,8 @@ struct HamlibBridge { std::atomic running{false}; jobject sink = nullptr; // global ref to HamlibNative.HamlibSink jmethodID onBytesToRig = nullptr; + int model_id = 0; // for logging only; set in nativeCreate, read in nativeRigOpen + bool opened = false; // rig_open() succeeded; nativeClose only rig_close()s then }; // ---- pump thread: hamlib's outbound CAT bytes -> Java sink ------------------ @@ -118,12 +120,23 @@ static HamlibBridge *handle_to_bridge(jlong h) { extern "C" { /* - * nativeOpen: build the bridge, init+open the rig on the loopback socket. - * Returns an opaque handle (>0) or 0 on failure. + * nativeCreate: build the bridge and start the pump thread, but do NOT call + * rig_open() yet. Returns an opaque handle (>0) or 0 on failure. + * + * Split from the open call (see nativeRigOpen below) so Java can publish the + * handle to HamlibRig.handle BEFORE rig_open()'s own synchronous handshake + * runs. HamlibRig.onReceiveData() only forwards radio bytes into the bridge + * when handle != 0 — with the original single nativeOpen(), handle was only + * set after it returned, so every byte the radio sent during rig_open()'s own + * handshake (its ID;/FA; probes) was silently dropped on the Java side, and + * hamlib always saw 0 chars and timed out, even against a radio that was + * actually replying. Confirmed on real hardware: serial.recv showed real + * traffic arriving while hamlib's own trace reported "Timed out ... after 0 + * chars" for the exact same window, every single time. */ JNIEXPORT jlong JNICALL -Java_com_k1af_ft8af_wave_HamlibNative_nativeOpen(JNIEnv *env, jclass, - jint modelId, jobject sink) { +Java_com_k1af_ft8af_wave_HamlibNative_nativeCreate(JNIEnv *env, jclass, + jint modelId, jobject sink) { ensure_vm(env); auto *b = new HamlibBridge(); @@ -173,30 +186,40 @@ Java_com_k1af_ft8af_wave_HamlibNative_nativeOpen(JNIEnv *env, jclass, close(b->listen_fd); delete b; return 0; } - // 5. open (connects to our listener; initial CAT handshake flows over the - // bridge to the real radio via Java) + b->model_id = modelId; + LOGI("hamlib bridge for model %d created on 127.0.0.1:%d (not yet opened)", + modelId, port); + return reinterpret_cast(b); +} + +/* + * nativeRigOpen: run rig_open() on an already-created bridge (see + * nativeCreate above for why this is split out). Java must publish the + * handle to HamlibRig.handle before calling this, so nativeFeedFromRig can + * deliver bytes the radio sends during rig_open()'s own handshake. + * + * Returns RIG_OK (0) on success, or a (negative) hamlib error code. Either + * way the bridge is left alive: the caller (HamlibRig) owns releasing it via + * nativeClose, exactly once, never concurrently with nativeFeedFromRig. A + * free done here on failure raced the serial read thread, which can still be + * inside nativeFeedFromRig(handle, …) on this same handle — the radio is + * actively replying to the handshake when rig_open() fails, so that race was + * not theoretical. + */ +JNIEXPORT jint JNICALL +Java_com_k1af_ft8af_wave_HamlibNative_nativeRigOpen(JNIEnv *, jclass, + jlong handle) { + auto *b = handle_to_bridge(handle); + if (b == nullptr) return -1; + int rc = rig_open(b->rig); if (rc != RIG_OK) { LOGE("rig_open failed: %d (%s)", rc, rigerror(rc)); - b->running = false; - // The pump thread may still be blocked in accept() (hamlib never - // connected). Close the listener FIRST to release accept(); if it did - // connect, shutting down srv_fd releases a blocked read(). Only then is - // pthread_join safe — otherwise it would hang forever. - int lfd = b->listen_fd.exchange(-1); - if (lfd >= 0) { shutdown(lfd, SHUT_RDWR); close(lfd); } - int sfd = b->srv_fd.load(); - if (sfd >= 0) shutdown(sfd, SHUT_RDWR); - pthread_join(b->pump, nullptr); - rig_cleanup(b->rig); - env->DeleteGlobalRef(b->sink); - sfd = b->srv_fd.load(); - if (sfd >= 0) close(sfd); - delete b; - return 0; + return rc; // caller still owns the bridge and frees it via nativeClose } - LOGI("hamlib rig %d opened via 127.0.0.1:%d", modelId, port); - return reinterpret_cast(b); + b->opened = true; + LOGI("hamlib rig %d opened", b->model_id); + return RIG_OK; } /* Feed bytes received from the real radio into hamlib's read side. */ @@ -273,13 +296,16 @@ Java_com_k1af_ft8af_wave_HamlibNative_nativeClose(JNIEnv *env, jclass, jlong han b->running = false; // Unblock the pump whether it is parked in accept() (never connected) or // read(): close the listener first, then shut down the accepted socket. - // Joining before this could deadlock. See nativeOpen's failure path. + // Joining before this could deadlock. See nativeRigOpen's failure path. int lfd = b->listen_fd.exchange(-1); if (lfd >= 0) { shutdown(lfd, SHUT_RDWR); close(lfd); } int sfd = b->srv_fd.load(); if (sfd >= 0) shutdown(sfd, SHUT_RDWR); if (b->pump) pthread_join(b->pump, nullptr); - if (b->rig) { rig_close(b->rig); rig_cleanup(b->rig); } + if (b->rig) { + if (b->opened) rig_close(b->rig); + rig_cleanup(b->rig); + } sfd = b->srv_fd.load(); if (sfd >= 0) close(sfd); if (b->sink) env->DeleteGlobalRef(b->sink); diff --git a/ft8af/app/src/main/java/com/k1af/ft8af/MainViewModel.java b/ft8af/app/src/main/java/com/k1af/ft8af/MainViewModel.java index 2ed58ac9f..02a6c63c2 100644 --- a/ft8af/app/src/main/java/com/k1af/ft8af/MainViewModel.java +++ b/ft8af/app/src/main/java/com/k1af/ft8af/MainViewModel.java @@ -133,6 +133,7 @@ import com.k1af.ft8af.timer.OnUtcTimer; import com.k1af.ft8af.timer.UtcTimer; import com.k1af.ft8af.ui.ToastMessage; +import com.k1af.ft8af.wave.AudioChannelSelect; import com.k1af.ft8af.wave.HamRecorder; import com.k1af.ft8af.wave.MicRecorder; import com.k1af.ft8af.wave.OnGetVoiceDataDone; @@ -988,13 +989,22 @@ private boolean needControlSco() {//determine whether SCO needs to be enabled ba baseRig != null && baseRig.supportWaveOverCAT()); } + // Whether the FT8 over's / the tune carrier's beginKeying() actually + // paused RX capture, so its endKeying() only resumes what it paused + // (see shouldPauseRxCaptureForTx's javadoc for why/when). One flag + // per keying source: arming TX only *requests* a tune stop, so the + // tune's endKeying() can run after the over's beginKeying(), and a + // single shared flag would let one consume the other's pause. + private volatile boolean rxPausedForOver = false; + private volatile boolean rxPausedForTune = false; + /** * Assert PTT (+ pause SCO) through the configured control path. Shared * by the FT8 TX path and the tune carrier (issue #408) so the * CAT/RTS/DTR branching lives in one place; with no control path * (VOX), this is a no-op and the audio itself keys the rig. */ - private void beginKeying() { + private void beginKeying(boolean tune) { boolean keysViaControlPath = GeneralVariables.controlMode == ControlMode.CAT || GeneralVariables.controlMode == ControlMode.RTS || GeneralVariables.controlMode == ControlMode.DTR; @@ -1026,10 +1036,31 @@ private void beginKeying() { pttSafetyLatch.onKeyed(); baseRig.setPTT(true); } + + // Last, so nothing above can throw between the pause and the + // caller's matching endKeying(): the TX audio itself only opens + // after the PTT settle delay, still well after this. + boolean pause = hamRecorder != null && shouldPauseRxCaptureForTx( + isQmxRig(baseRig), + hamRecorder.isMicSource(), + hamRecorder.getMicRecorder().isUsingUsbDirect(), + GeneralVariables.audioOutputDeviceId == -1, + GeneralVariables.usbAudioInputVendorId, + GeneralVariables.usbAudioInputProductId, + GeneralVariables.usbAudioOutputVendorId, + GeneralVariables.usbAudioOutputProductId); + if (pause) { + hamRecorder.pauseForTx(); + } + if (tune) { + rxPausedForTune = pause; + } else { + rxPausedForOver = pause; + } } - /** Release PTT (+ restore SCO); counterpart of {@link #beginKeying()}. */ - private void endKeying() { + /** Release PTT (+ restore SCO); counterpart of {@link #beginKeying(boolean)}. */ + private void endKeying(boolean tune) { if (GeneralVariables.controlMode == ControlMode.CAT || GeneralVariables.controlMode == ControlMode.RTS || GeneralVariables.controlMode == ControlMode.DTR) { @@ -1047,11 +1078,23 @@ private void endKeying() { // The post-TX restart has been requested; the next over takes a // fresh snapshot in beginKeying(). txScoLatch.keyUp(); + + boolean paused; + if (tune) { + paused = rxPausedForTune; + rxPausedForTune = false; + } else { + paused = rxPausedForOver; + rxPausedForOver = false; + } + if (paused) { + hamRecorder.resumeAfterTx(); + } } @Override public void onBeforeTransmit(Ft8Message message, int functionOder) { - beginKeying(); + beginKeying(false); if (ft8TransmitSignal.isActivated()) { GeneralVariables.transmitMessages.add(message); //mutableTransmitMessages.postValue(GeneralVariables.transmitMessages); @@ -1061,17 +1104,17 @@ public void onBeforeTransmit(Ft8Message message, int functionOder) { @Override public void onAfterTransmit(Ft8Message message, int functionOder) { - endKeying(); + endKeying(false); } @Override public void onTuneKeyDown() { - beginKeying(); + beginKeying(true); } @Override public void onTuneKeyUp() { - endKeying(); + endKeying(true); } @Override @@ -2352,6 +2395,100 @@ private void repairCivAddressAgainstModel() { } } + /** + * The QRP Labs QMX needs mono/left-channel TX audio to fully drive its + * modulator -- confirmed on real hardware across three independent apps + * (WSJT-X, QFT8, and this one): FT8AF's historical {@code BOTH} default + * opens a mono {@code AudioTrack} and leaves the mono-to-stereo upmix to + * Android/the USB Audio Class device, with no control over how much gain + * that upmix applies per channel, while an explicit {@code LEFT} + * selection opens a genuine stereo track with the left channel at full + * scale and the right silenced (see {@link AudioChannelSelect}) -- + * underdriven either way until the operator finds the manual Settings + * toggle. While a QMX is the connected rig, uses {@code LEFT} for TX, but + * only when the operator has never touched that setting at all (no + * "txAudioChannel" config row) -- an explicit choice, even {@code BOTH} + * itself, is never overridden. Applied in memory on every rig connect and + * never saved, so switching to another rig goes back to {@code BOTH} + * instead of leaving that rig on a left-only TX it never asked for. + */ + private void applyTxChannelDefaultForRig(boolean rigIsQmx) { + if (databaseOpr == null) return; + boolean keyExists = databaseOpr.hasConfigKey(AudioChannelSelect.TX_CONFIG_KEY); + int channel = defaultTxChannelForRig(rigIsQmx, keyExists, GeneralVariables.txAudioChannel); + if (channel != GeneralVariables.txAudioChannel) { + GeneralVariables.txAudioChannel = channel; + GeneralVariables.fileLog("TX audio channel defaulted to " + + (channel == AudioChannelSelect.LEFT ? "LEFT (QMX)" : "BOTH") + + " (no saved user choice)"); + } + } + + /** + * The decision behind {@link #applyTxChannelDefaultForRig}, pure and + * testable without a real {@code DatabaseOpr}/{@code Context}: the TX + * channel to use for the rig being connected. A saved choice + * ({@code txChannelConfigKeyExists}) keeps {@code current}; otherwise the + * QMX gets {@code LEFT} and every other rig the {@code BOTH} default. + */ + static int defaultTxChannelForRig(boolean rigIsQmx, boolean txChannelConfigKeyExists, + int current) { + if (txChannelConfigKeyExists) return current; + return rigIsQmx ? AudioChannelSelect.LEFT : AudioChannelSelect.BOTH; + } + + /** + * Whether {@code rig} is a QRP Labs QMX, through either rig-list entry: the + * hamlib backend or the native {@link QrpLabsQmxRig}. The RX pause and the + * LEFT TX channel default work around the radio's USB audio, so they apply + * whichever CAT driver talks to it. + */ + static boolean isQmxRig(BaseRig rig) { + return rig instanceof QrpLabsQmxRig + || (rig instanceof HamlibRig && ((HamlibRig) rig).isQmx()); + } + + /** + * Whether RX capture must be paused for the duration of this TX (see + * {@link HamRecorder#pauseForTx()} for the real-hardware failure this + * works around). True only for the QRP Labs QMX, and only when RX is + * actually holding an Android capture session on the exact same USB + * device TX is about to play on. The QMX gate matters: matching VID:PID + * alone is true for every single-cable USB rig (IC-7300, IC-705, + * IC-9700, FT-991A, Digirig, SignaLink...) whose operator picked the + * rig's codec for both directions, and pausing those would blank the + * waterfall during TX, break full-duplex satellite monitoring, and risk + * delaying the last RX slot's decode -- for a HAL conflict only ever + * confirmed with a QMX: + * + *
    + *
  • the rig must be a QMX ({@link #isQmxRig}); + *
  • RX must be the phone-audio path ({@code HamRecorder.isMicSource()}), + * not a LAN-connected rig -- those never touch this HAL at all, and + * toggling capture off would spuriously gate their audio too (see + * {@code HamRecorder.doOnWaveDataReceived}'s {@code isRunning} check); + *
  • RX must not already be USB-direct (raw libusb capture, a + * different HAL path with no ALSA proxy to conflict over); + *
  • TX must not be USB-direct output either (that path is separately, + * already broken -- {@code LIBUSB_ERROR_IO} -- and conflating the + * two bugs would make either one harder to fix); + *
  • the input and output VID:PID must both be set and match -- two + * different USB devices, or a non-USB default mic/speaker, have no + * reason to share this HAL's single ALSA card. + *
+ */ + static boolean shouldPauseRxCaptureForTx(boolean rigIsQmx, boolean rxIsMicSource, + boolean rxIsUsbDirect, boolean txIsUsbDirectOutput, + int usbInputVendorId, int usbInputProductId, + int usbOutputVendorId, int usbOutputProductId) { + if (!rigIsQmx || !rxIsMicSource || rxIsUsbDirect || txIsUsbDirectOutput) { + return false; + } + return usbInputVendorId != 0 + && usbInputVendorId == usbOutputVendorId + && usbInputProductId == usbOutputProductId; + } + /** * Create different rig models based on the instruction set */ @@ -2449,12 +2586,23 @@ private void connectRig() { case InstructionSet.KENWOOD_TS440: baseRig = new KenwoodTS440Rig();//KENWOOD TS-440S (TS-570 CAT, USB mode) break; - case InstructionSet.HAMLIB: + case InstructionSet.HAMLIB: { // hamlib model number is carried in the rig table's address column - // (parsed base-16), e.g. FT-891 = 1036 = 0x40C. - baseRig = new HamlibRig(GeneralVariables.civAddress); + // (parsed base-16), e.g. FT-891 = 1036 = 0x40C. Derived fresh from the + // selected rig entry (HamlibRig.modelIdFor), not persisted separately -- + // a separate "hamlibModel" config key existed briefly but could go stale + // or hold a non-hamlib address (civAddress's own persistence is byte- + // masked, a real ICOM CI-V address being a byte where a hamlib model ID + // routinely isn't, so it can't carry this value either). + android.content.Context ctx = GeneralVariables.getMainContext(); + RigNameList.RigName selected = ctx == null ? null + : RigNameList.getInstance(ctx).getRigNameByIndex(GeneralVariables.modelNo); + int hamlibModelId = HamlibRig.modelIdFor(selected); + baseRig = new HamlibRig(hamlibModelId); break; + } } + applyTxChannelDefaultForRig(isQmxRig(baseRig)); // Store the rig name for PSKReporter software string. // Use the user-selected model name from RigNameList (same source as the diff --git a/ft8af/app/src/main/java/com/k1af/ft8af/database/DatabaseOpr.java b/ft8af/app/src/main/java/com/k1af/ft8af/database/DatabaseOpr.java index e333c30a3..6ee67e168 100644 --- a/ft8af/app/src/main/java/com/k1af/ft8af/database/DatabaseOpr.java +++ b/ft8af/app/src/main/java/com/k1af/ft8af/database/DatabaseOpr.java @@ -800,6 +800,23 @@ public void writeConfig(String KeyName, String Value, OnAfterWriteConfig onAfter new WriteConfig(db, KeyName, Value, onAfterWriteConfig).execute(); } + /** + * Whether a config row exists for {@code keyName} at all -- synchronous + * (unlike every other config accessor here), for callers on a background + * thread that need an answer before deciding what to do next (e.g. + * whether the operator has ever touched a given setting themselves, + * distinct from it merely holding its hardcoded default). + */ + public boolean hasConfigKey(String keyName) { + Cursor cursor = db.rawQuery("select 1 from config where KeyName = ? limit 1", + new String[]{keyName}); + try { + return cursor.moveToFirst(); + } finally { + cursor.close(); + } + } + /** * The config value as it may appear in logcat. Keys that hold credentials * ({@link SettingsBackup#getSENSITIVE_KEYS()} -- the API keys and passwords a diff --git a/ft8af/app/src/main/java/com/k1af/ft8af/ft8transmit/FT8TransmitSignal.java b/ft8af/app/src/main/java/com/k1af/ft8af/ft8transmit/FT8TransmitSignal.java index 40fd19c88..2047a3e78 100644 --- a/ft8af/app/src/main/java/com/k1af/ft8af/ft8transmit/FT8TransmitSignal.java +++ b/ft8af/app/src/main/java/com/k1af/ft8af/ft8transmit/FT8TransmitSignal.java @@ -1112,7 +1112,14 @@ private void playViaAudioTrack(float[] buffer) { // set preferred output device if (GeneralVariables.audioOutputDeviceId > 0) { - audioTrack.setPreferredDevice(preferredOutputDevice); // null resets to default + boolean ok = audioTrack.setPreferredDevice(preferredOutputDevice); // null resets to default + // Logs the REQUESTED id (what was persisted) and the RESOLVED one separately -- + // see MicRecorder's matching reinit log for why these can legitimately differ. + GeneralVariables.fileLog(String.format( + "playFT8Signal: AudioTrack preferredDevice requestedId=%d resolvedId=%d ok=%b deviceFound=%b", + GeneralVariables.audioOutputDeviceId, + preferredOutputDevice == null ? -1 : preferredOutputDevice.getId(), + ok, preferredOutputDevice != null)); } else { applyDefaultOutputRoutingOverride(audioTrack); } @@ -3312,7 +3319,12 @@ private void playTuneTone() { int bufBytes = Math.max(targetBufBytes, minBuf > 0 ? minBuf : targetBufBytes); track = buildTxAudioTrack(tuneAttributes, tuneFormat, bufBytes, "TUNE"); if (GeneralVariables.audioOutputDeviceId > 0) { - track.setPreferredDevice(tuneOutputDevice); + boolean ok = track.setPreferredDevice(tuneOutputDevice); + GeneralVariables.fileLog(String.format( + "TUNE: AudioTrack preferredDevice requestedId=%d resolvedId=%d ok=%b deviceFound=%b", + GeneralVariables.audioOutputDeviceId, + tuneOutputDevice == null ? -1 : tuneOutputDevice.getId(), + ok, tuneOutputDevice != null)); } else { applyDefaultOutputRoutingOverride(track); } diff --git a/ft8af/app/src/main/java/com/k1af/ft8af/rigs/HamlibRig.java b/ft8af/app/src/main/java/com/k1af/ft8af/rigs/HamlibRig.java index ae1f358ab..cbbcc887f 100644 --- a/ft8af/app/src/main/java/com/k1af/ft8af/rigs/HamlibRig.java +++ b/ft8af/app/src/main/java/com/k1af/ft8af/rigs/HamlibRig.java @@ -1,13 +1,16 @@ package com.k1af.ft8af.rigs; +import android.os.SystemClock; import android.util.Log; import com.k1af.ft8af.connector.BaseRigConnector; +import com.k1af.ft8af.database.RigNameList; import com.k1af.ft8af.wave.HamlibNative; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.locks.ReentrantReadWriteLock; /** * A {@link BaseRig} backed by hamlib instead of a hand-rolled per-manufacturer @@ -27,12 +30,47 @@ public class HamlibRig extends BaseRig { private static final String TAG = "HamlibRig"; - /** hamlib mode for FT8/FT4: DATA-USB, so audio routes through the data path. */ + /** + * hamlib mode for FT8/FT4 on most rigs: DATA-USB, so audio routes through + * the data path rather than a mic/voice path. + */ private static final String FT_MODE = "PKTUSB"; + /** + * QMX override: hamlib's generic Kenwood mode table (rigs/kenwood/kenwood.c's + * kenwood_mode_table) encodes RIG_MODE_PKTUSB as table index 13, which + * kenwood_set_mode() turns into the ASCII letter 'D' -- "MDD;". The QMX's own + * CAT manual (cat_1_04_003.pdf) only accepts single-digit MD values + * (1,2,3,5,6,7,8,9) and silently rejects 'D', so PKTUSB never actually set the + * mode on this radio. "RTTY" is table index 6 (RIG_MODE_RTTY, <= 9, encoded + * as a literal digit "MD6;"), the QMX manual's own "6 (FSK)" -- its real Digi + * mode. Applying this to every hamlib rig (not just the QMX) would send + * RIG_MODE_RTTY to rigs that expect PKTUSB, e.g. an FT-891 via newcat would + * get "MD06" (RTTY-LSB) instead of DATA-USB -- wrong mode and filter. + */ + private static final String QMX_FT_MODE = "RTTY"; + + /** Which {@link #FT_MODE} string to send for a given hamlib model id. */ + static String ftModeFor(int modelId) { + return modelId == QRPLABS_QMX_MODEL_ID ? QMX_FT_MODE : FT_MODE; + } + private final int modelId; + private final NativeApi nativeApi; + /** Clock behind the open-retry backoff; see {@link #ensureOpen()}. */ + private final java.util.function.LongSupplier elapsedRealtimeMs; private final ExecutorService worker = Executors.newSingleThreadExecutor(); private volatile long handle = 0; + /** + * Guards {@link #handle} against the one genuine cross-thread race: a free + * ({@link #releaseHandle()}, on the single {@link #worker} thread) running + * concurrently with a feed ({@link #onReceiveData}, on the serial read + * thread) on the same native handle. {@code ensureOpen()}/{@code + * releaseHandle()} never race each other -- both only ever run via {@link + * #worker}, a single-thread executor -- so publishing a freshly opened + * handle needs no lock; only freeing it does. + */ + private final ReentrantReadWriteLock bridgeLock = new ReentrantReadWriteLock(); /** * Set once in {@link #onDisconnecting()}. After that the {@link #worker} * executor has been shut down, so any further {@code worker.submit(...)} @@ -51,7 +89,112 @@ public class HamlibRig extends BaseRig { }; public HamlibRig(int modelId) { + this(modelId, REAL_NATIVE_API); + } + + /** Visible for tests: inject a fake native layer to drive races on the JVM. */ + HamlibRig(int modelId, NativeApi nativeApi) { + this(modelId, nativeApi, SystemClock::elapsedRealtime); + } + + /** + * Visible for tests: also inject the clock behind the open-retry backoff + * (see {@link #ensureOpen()}), so it's testable deterministically rather + * than depending on real elapsed time. {@code SystemClock.elapsedRealtime()} + * is otherwise unmocked and returns 0 under a plain (non-Robolectric) JVM + * unit test (app/build.gradle's {@code unitTests.returnDefaultValues}) -- + * harmless on its own, but worth not depending on at all here. + */ + HamlibRig(int modelId, NativeApi nativeApi, java.util.function.LongSupplier elapsedRealtimeMs) { this.modelId = modelId; + this.nativeApi = nativeApi; + this.elapsedRealtimeMs = elapsedRealtimeMs; + } + + /** + * Seam over {@link HamlibNative}'s static natives. {@code HamlibNative} + * can't be exercised from a JVM unit test (no {@code libft8af.so}, and + * {@link HamlibNative#isAvailable()} is hard-wired to {@code false} there), + * so real races like findings 3/4 (use-after-free on a failed open, + * double-free/leak on a disconnect that overlaps an open) were otherwise + * untestable outside real hardware. A fake implementation lets tests drive + * {@link #ensureOpen()}/{@link #releaseHandle()} directly. + */ + interface NativeApi { + boolean isAvailable(); + long nativeCreate(int modelId, HamlibNative.HamlibSink sink); + int nativeRigOpen(long handle); + void nativeFeedFromRig(long handle, byte[] data); + int nativeSetFreq(long handle, long hz); + long nativeGetFreq(long handle); + int nativeSetMode(long handle, String mode); + int nativeSetPtt(long handle, boolean on); + void nativeClose(long handle); + } + + private static final NativeApi REAL_NATIVE_API = new NativeApi() { + @Override public boolean isAvailable() { return HamlibNative.isAvailable(); } + @Override public long nativeCreate(int modelId, HamlibNative.HamlibSink sink) { + return HamlibNative.nativeCreate(modelId, sink); + } + @Override public int nativeRigOpen(long handle) { return HamlibNative.nativeRigOpen(handle); } + @Override public void nativeFeedFromRig(long handle, byte[] data) { + HamlibNative.nativeFeedFromRig(handle, data); + } + @Override public int nativeSetFreq(long handle, long hz) { return HamlibNative.nativeSetFreq(handle, hz); } + @Override public long nativeGetFreq(long handle) { return HamlibNative.nativeGetFreq(handle); } + @Override public int nativeSetMode(long handle, String mode) { + return HamlibNative.nativeSetMode(handle, mode); + } + @Override public int nativeSetPtt(long handle, boolean on) { return HamlibNative.nativeSetPtt(handle, on); } + @Override public void nativeClose(long handle) { HamlibNative.nativeClose(handle); } + }; + + /** + * Minimum time between native open attempts after one fails (either + * {@code nativeCreate} or {@code nativeRigOpen}/the CAT handshake). + * Without this, a persistently unresponsive radio got a brand-new native + * bridge (loopback socket + pump thread) on every single CAT call that + * happens to run while handle==0 -- confirmed on real hardware: a QMX + * stuck in a non-CAT mode got 354 fresh bridges in about 17 minutes (each + * one created and torn down within ~300ms, via the ~3s CAT-liveness + * watchdog tick plus other CAT calls queuing in between). Each cycle was + * cleaned up correctly (no leak -- see releaseHandle()), but churning + * native resources this often for an indefinitely unresponsive radio is + * still wasteful. 15s caps that at roughly a quarter of the previous rate + * while still recovering promptly once the radio answers again. + */ + private static final long OPEN_RETRY_BACKOFF_MS = 15_000; + /** + * {@link #elapsedRealtimeMs}'s reading at the last open failure, or + * {@link Long#MIN_VALUE} as the "never failed" sentinel -- NOT 0: a plain + * JVM unit test (no Robolectric) gets 0 for every unmocked {@code + * SystemClock.elapsedRealtime()} call, which collided with 0 as a + * sentinel and made the very first backoff check after a failure see + * "now == last failure == 0" and wrongly allow an immediate retry. + */ + private volatile long lastOpenFailureAtMs = Long.MIN_VALUE; + + /** + * Whether {@link #ensureOpen()} should attempt a fresh native open now, + * given when the last attempt failed. Pure/testable: {@code + * ensureOpen()} itself touches native calls and a connector, so the + * backoff decision is extracted here rather than timed with a real + * clock + sleep in a test. + * + * @param nowMs current time; callers must use {@link + * SystemClock#elapsedRealtime()}, not wall-clock + * time -- an NTP sync or manual clock change could + * otherwise stall reopening indefinitely (clock + * jumps back) or skip the backoff entirely (clock + * jumps forward) + * @param lastFailureAtMs the same clock's reading when the last attempt + * failed, or {@link Long#MIN_VALUE} if there + * hasn't been one (NOT 0 -- see {@link + * #lastOpenFailureAtMs}'s javadoc) + */ + static boolean shouldAttemptOpen(long nowMs, long lastFailureAtMs, long backoffMs) { + return lastFailureAtMs == Long.MIN_VALUE || nowMs - lastFailureAtMs >= backoffMs; } /** @@ -71,20 +214,121 @@ private synchronized boolean ensureOpen() { if (handle != 0) { return true; } + if (!shouldAttemptOpen(elapsedRealtimeMs.getAsLong(), lastOpenFailureAtMs, OPEN_RETRY_BACKOFF_MS)) { + return false; + } BaseRigConnector c = getConnector(); - if (c == null || !c.isConnected() || !HamlibNative.isAvailable()) { + if (c == null || !c.isConnected() || !nativeApi.isAvailable()) { + return false; + } + if (modelId <= 0) { + // Never hand hamlib a placeholder/unset model ID: rig_init(0) was + // observed on real hardware to abort the whole process (native + // SIGABRT, "pthread_mutex_lock called on a destroyed mutex") a + // fraction of a second later, not just fail gracefully. 0 means + // "not a hamlib entry" (see HamlibRig.modelIdFor) and must never + // reach native code. + Log.w(TAG, "ensureOpen: refusing to open invalid hamlib model " + modelId); return false; } - long h = HamlibNative.nativeOpen(modelId, sink); + long h = nativeApi.nativeCreate(modelId, sink); if (h == 0) { - Log.w(TAG, "nativeOpen failed for hamlib model " + modelId); + Log.w(TAG, "nativeCreate failed for hamlib model " + modelId); + lastOpenFailureAtMs = elapsedRealtimeMs.getAsLong(); return false; } + // Publish the handle BEFORE running the handshake: onReceiveData() + // only forwards radio bytes into the bridge once handle != 0, and + // rig_open() (called by nativeRigOpen below) does its own synchronous + // CAT handshake that needs those bytes fed back in real time. With a + // single combined open call, every byte the radio sent during that + // handshake was silently dropped (handle was still 0), so hamlib + // always saw a timeout — confirmed on real hardware: the radio was + // genuinely replying (serial.recv had real traffic) while hamlib's + // own trace reported "0 chars" for that exact window, every time. handle = h; + int rc = nativeApi.nativeRigOpen(h); + if (rc != 0) { + Log.w(TAG, "nativeRigOpen failed for hamlib model " + modelId + " rc=" + rc); + releaseHandle(); + lastOpenFailureAtMs = elapsedRealtimeMs.getAsLong(); + return false; + } Log.i(TAG, "hamlib model " + modelId + " opened"); + lastOpenFailureAtMs = Long.MIN_VALUE; + disableQmxIqModeIfApplicable(c); return true; } + /** + * Unpublish and free the bridge. Runs on {@link #worker} (so it never + * overlaps an {@link #ensureOpen()} on the same handle — both only ever + * run there) under {@link #bridgeLock}'s write lock (so it never overlaps + * a feed in {@link #onReceiveData}, which takes the read lock). + */ + private void releaseHandle() { + bridgeLock.writeLock().lock(); + try { + long h = handle; + handle = 0; + if (h != 0) nativeApi.nativeClose(h); + } finally { + bridgeLock.writeLock().unlock(); + } + } + + /** + * QRP Labs QMX-specific fixup, sent once right after a successful open: + * force I/Q mode off ({@code "Q90;"}). hamlib has no generic API for this + * -- it's one of the QMX's vendor "Q" extended commands + * (qrp-labs.com/images/qmx/manuals/cat_1_04_003.pdf) -- so this bypasses + * hamlib and writes the connector directly, the same mechanism + * {@link #sink} uses for hamlib's own bytes. Left on (e.g. from prior SDR + * use via the radio's own front-panel menu), I/Q mode streams raw ADC + * samples instead of demodulated audio, which a decoder sees as noise; + * the setting is session-only, so it silently returns on every power- + * cycle. Matches danleetw/FT8TW#78, fixed there with the same command. + * + *

Package-private: visible for {@code disableQmxIqModeIfApplicable_*} tests. + * + * @param c the connector to write to; never null when called from + * {@link #ensureOpen()} (already checked non-null/connected) + */ + void disableQmxIqModeIfApplicable(BaseRigConnector c) { + if (modelId != QRPLABS_QMX_MODEL_ID) return; + c.sendData("Q90;".getBytes(java.nio.charset.StandardCharsets.US_ASCII)); + } + + /** True when this rig is the QRP Labs QMX (see {@link #QRPLABS_QMX_MODEL_ID}). */ + public boolean isQmx() { + return modelId == QRPLABS_QMX_MODEL_ID; + } + + /** + * hamlib model ID for the QRP Labs QMX (0x809), per rigaddress.txt. Public: + * also used by {@code MainViewModel} to apply the QMX's LEFT-channel TX + * audio default on connect (see {@code defaultTxChannelForRig}). + */ + public static final int QRPLABS_QMX_MODEL_ID = 0x809; + + /** + * hamlib model ID for the currently-selected rig entry, or 0 if it isn't a + * hamlib entry at all. Derives it fresh from {@code rigaddress.txt}'s + * address column rather than from a separately persisted config value: a + * prior {@code hamlibModel} config key was written for every rig pick + * (including non-hamlib ones, storing e.g. an ICOM CI-V address like 0x94 + * there) and left stale by one picker path that never wrote it at all, so + * a leftover or stale value could silently win over the actually-selected + * entry after a restart and open the wrong hamlib backend. + * + * @param selected the picked rig entry (e.g. from + * {@code RigNameList.getRigNameByIndex}), or null + */ + public static int modelIdFor(RigNameList.RigName selected) { + if (selected == null || selected.instructionSet != InstructionSet.HAMLIB) return 0; + return Math.max(selected.address, 0); + } + @Override public boolean isConnected() { BaseRigConnector c = getConnector(); @@ -94,9 +338,17 @@ public boolean isConnected() { @Override public void onReceiveData(byte[] data) { // Called on the serial read thread — a quick, non-blocking socket write. - long h = handle; - if (h != 0) { - HamlibNative.nativeFeedFromRig(h, data); + // Read-locked against releaseHandle()'s write lock: without this, a feed + // here could run concurrently with the native free, using a handle whose + // bridge is mid-teardown (or already deleted) on the worker thread. + bridgeLock.readLock().lock(); + try { + long h = handle; + if (h != 0) { + nativeApi.nativeFeedFromRig(h, data); + } + } finally { + bridgeLock.readLock().unlock(); } } @@ -138,8 +390,9 @@ void awaitWorkerForTest() throws InterruptedException { public void setUsbModeToRig() { submitToWorker(() -> { if (!ensureOpen()) return; - int rc = HamlibNative.nativeSetMode(handle, FT_MODE); - if (rc != 0) Log.w(TAG, "set_mode(" + FT_MODE + ") rc=" + rc); + String mode = ftModeFor(modelId); + int rc = nativeApi.nativeSetMode(handle, mode); + if (rc != 0) Log.w(TAG, "set_mode(" + mode + ") rc=" + rc); }); } @@ -151,7 +404,7 @@ public void setFreqToRig() { final long hz = getFreq(); submitToWorker(() -> { if (!ensureOpen()) return; - int rc = HamlibNative.nativeSetFreq(handle, hz); + int rc = nativeApi.nativeSetFreq(handle, hz); if (rc != 0) Log.w(TAG, "set_freq(" + hz + ") rc=" + rc); }); } @@ -160,7 +413,7 @@ public void setFreqToRig() { public void readFreqFromRig() { submitToWorker(() -> { if (!ensureOpen()) return; - long f = HamlibNative.nativeGetFreq(handle); + long f = nativeApi.nativeGetFreq(handle); if (f > 0) { setFreq(f); // updates dial + fires CAT-liveness (BaseRig) } @@ -172,7 +425,7 @@ public void setPTT(boolean on) { super.setPTT(on); // notify listeners / meter state submitToWorker(() -> { if (!ensureOpen()) return; - int rc = HamlibNative.nativeSetPtt(handle, on); + int rc = nativeApi.nativeSetPtt(handle, on); if (rc != 0) Log.w(TAG, "set_ptt(" + on + ") rc=" + rc); }); } @@ -191,22 +444,22 @@ public void onDisconnecting() { // shutdown — would throw RejectedExecutionException and crash the app. if (closed) return; closed = true; - final long h = handle; - handle = 0; try { - worker.submit(() -> { - if (h != 0) { - HamlibNative.nativeClose(h); - } - }); + // Read/clear/close handle via releaseHandle() on the SAME worker + // thread ensureOpen() runs on, queued behind it rather than racing + // it from this (caller) thread. The previous version read/cleared + // the volatile `handle` field directly here, concurrently with the + // worker possibly being mid-ensureOpen(): depending on timing that + // either leaked the bridge ensureOpen was about to publish (this + // close captures a stale value, e.g. 0, just before ensureOpen + // overwrites it) or closed a handle a subsequent ensureOpen() had + // already reused after wrongly seeing a momentarily-zeroed `handle` + // as "not yet open" and recreating it. + worker.submit(this::releaseHandle); } catch (RejectedExecutionException e) { - // Worker already terminated, so it can't run the close for us. The - // handle is still reachable via the local h — close it inline rather - // than leaking the native rig/pump resources. + // Worker already terminated, so it can't run the release for us. Log.w(TAG, "worker rejected close task; closing handle inline"); - if (h != 0) { - HamlibNative.nativeClose(h); - } + releaseHandle(); } worker.shutdown(); } diff --git a/ft8af/app/src/main/java/com/k1af/ft8af/wave/HamRecorder.java b/ft8af/app/src/main/java/com/k1af/ft8af/wave/HamRecorder.java index 5ba3ddbf8..2779f7a96 100644 --- a/ft8af/app/src/main/java/com/k1af/ft8af/wave/HamRecorder.java +++ b/ft8af/app/src/main/java/com/k1af/ft8af/wave/HamRecorder.java @@ -231,6 +231,78 @@ public void stopRecord() { isRunning = false; } + // Nesting state for pauseForTx()/resumeAfterTx(); see TxPauseState. + private final TxPauseState txPause = new TxPauseState(); + + /** + * Release RX capture for the duration of one TX "over"/TUNE key-down, when + * the caller ({@code MainViewModel.shouldPauseRxCaptureForTx}) has decided + * TX and RX share the same physical USB audio device. On that hardware, + * opening TX playback while this class's RX capture is still open on the + * same device fails at the ALSA proxy level + * (alsa_device_proxy: proxy_open() pcm_is_ready() failed: cannot + * set hw params: I/O error) and every subsequent write is silently + * dropped (AudioALSAPlaybackHandlerUsb: write(), mPcm is NULL!) + * -- confirmed on real hardware: FT8AF's generated waveform measured + * full-scale all the way to the {@code AudioTrack.write()} call, and TX + * was still inaudible on the rig, because the HAL never actually opened + * the output. A reference app (QFT8) that drives the same radio to full + * output does exactly this: close capture, play, reopen capture + * (confirmed via its own logcat -- it closes {@code + * AudioALSACaptureHandlerUsb} ~260ms before opening TX and reopens it + * right after). + * + *

Counterpart: {@link #resumeAfterTx()}. + */ + public synchronized void pauseForTx() { + if (txPause.onPause(isRunning)) { + stopRecord(); + } + } + + /** + * Restart RX capture after a TX "over"/TUNE key-up, undoing {@link + * #pauseForTx()} -- but only once every outstanding pause has been + * resumed, and only when the outermost pause actually stopped a running + * capture, so a TX that happens while the operator had RX off does not + * unexpectedly turn capture back on. A resume with no pause outstanding + * is ignored. + */ + public synchronized void resumeAfterTx() { + if (txPause.onResume()) { + startRecord(); + } + } + + /** + * Nesting state behind {@link #pauseForTx()}/{@link #resumeAfterTx()}, + * pure so it is testable without a recorder. A tune carrier and an FT8 + * over can overlap (arming TX only requests the tune stop), so pauses + * nest: only the first stops capture, and only the last resume restarts + * it -- and only if that first pause actually stopped a running capture, + * never reviving one the operator had deliberately stopped. + */ + static final class TxPauseState { + private int depth = 0; + private boolean wasRunning = false; + + /** A pause began; true when the caller must stop the running capture. */ + boolean onPause(boolean running) { + if (depth++ > 0) return false; + wasRunning = running; + return running; + } + + /** A pause ended; true when the caller must restart capture. */ + boolean onResume() { + if (depth == 0) return false; + if (--depth > 0) return false; + boolean restart = wasRunning; + wasRunning = false; + return restart; + } + } + /** The underlying mic capture, for routing introspection (issue #759). */ public MicRecorder getMicRecorder() { return micRecorder; diff --git a/ft8af/app/src/main/java/com/k1af/ft8af/wave/HamlibNative.java b/ft8af/app/src/main/java/com/k1af/ft8af/wave/HamlibNative.java index 0097effab..498675a8e 100644 --- a/ft8af/app/src/main/java/com/k1af/ft8af/wave/HamlibNative.java +++ b/ft8af/app/src/main/java/com/k1af/ft8af/wave/HamlibNative.java @@ -51,13 +51,32 @@ public interface HamlibSink { } /** - * Init + open a hamlib rig, wiring its transport to {@code sink}. + * Init a hamlib rig and its loopback-socket bridge, wiring its transport + * to {@code sink}, but do NOT perform the CAT handshake yet (see + * {@link #nativeRigOpen}). Split so the caller can publish the returned + * handle (e.g. to {@code HamlibRig.handle}) before the handshake runs — + * {@link #nativeFeedFromRig} silently drops bytes delivered against a + * handle the caller hasn't published yet, which previously meant every + * byte the radio sent during the handshake itself was lost, and hamlib + * always saw a timeout even against a replying radio. * * @param modelId hamlib rig model number (e.g. 3085 = IC-705) * @param sink receives CAT bytes bound for the radio * @return opaque handle (> 0) for the other calls, or 0 on failure */ - public static native long nativeOpen(int modelId, HamlibSink sink); + public static native long nativeCreate(int modelId, HamlibSink sink); + + /** + * Run hamlib's {@code rig_open()} (the CAT handshake) on a bridge handle + * from {@link #nativeCreate}. The caller must have already published the + * handle so inbound bytes during the handshake reach {@link #nativeFeedFromRig}. + * + * @return 0 (RIG_OK) on success, or a hamlib error code. Either way the + * bridge is left alive; the caller must release it with + * {@link #nativeClose} exactly once (success or failure), never + * concurrently with {@link #nativeFeedFromRig}. + */ + public static native int nativeRigOpen(long handle); /** Push bytes received from the radio into hamlib's read side. */ public static native void nativeFeedFromRig(long handle, byte[] data); diff --git a/ft8af/app/src/main/java/com/k1af/ft8af/wave/MicRecorder.java b/ft8af/app/src/main/java/com/k1af/ft8af/wave/MicRecorder.java index 6dac1b88a..813273ec8 100644 --- a/ft8af/app/src/main/java/com/k1af/ft8af/wave/MicRecorder.java +++ b/ft8af/app/src/main/java/com/k1af/ft8af/wave/MicRecorder.java @@ -712,13 +712,31 @@ public void run() { * restart capture the operator explicitly stopped. {@link #reinitialize()} * itself does the equivalent teardown inline rather than calling this, so * its own internal stop/rebuild cycle never clears that intent flag. + * + *

Retires the AudioRecord reader thread before returning (new + * generation, stop the recording to unblock its read(), bounded join), + * so a {@link #start()} right after -- e.g. RX resuming after a short TX + * pause -- can't leave the old reader running alongside the new one on + * the same AudioRecord (a fatal native abort), or let the old reader's + * exit stop the new recording. */ public synchronized void stopRecord() { + captureGeneration++; isRunning = false; shouldBeRunning = false; if (useUsbAudio && usbAudioDevice != null) { usbAudioDevice.stopCapture(); } + if (audioRecord != null) { + try { + if (audioRecord.getRecordingState() == AudioRecord.RECORDSTATE_RECORDING) { + audioRecord.stop(); + } + } catch (Exception e) { + Log.d(TAG, "stopRecord: error stopping AudioRecord: " + e.getMessage()); + } + joinCaptureThread(); + } } public OnDataListener getOnDataListener() { @@ -817,7 +835,17 @@ public synchronized void reinitialize() { if (audioRecord != null && GeneralVariables.audioInputDeviceId > 0) { AudioDeviceInfo deviceInfo = findAudioDeviceById( GeneralVariables.audioInputDeviceId, AudioManager.GET_DEVICES_INPUTS); - audioRecord.setPreferredDevice(deviceInfo); + boolean ok = audioRecord.setPreferredDevice(deviceInfo); + // Logs the REQUESTED id (what was persisted) and the RESOLVED one + // separately: findAudioDeviceById() can fall back to a different, + // currently-live id via UsbAudioDevice.resolveDeviceIndex's product-name + // match when the requested one went stale across a USB re-enumeration -- + // confirmed on real hardware (a QMX power-cycle), where logging only the + // requested id made a successful fallback look identical to an exact hit. + GeneralVariables.fileLog(String.format( + "MicRecorder: reinit preferredDevice requestedId=%d resolvedId=%d ok=%b deviceFound=%b", + GeneralVariables.audioInputDeviceId, + deviceInfo == null ? -1 : deviceInfo.getId(), ok, deviceInfo != null)); } } @@ -841,6 +869,16 @@ static boolean shouldRestartAfterReinitialize(boolean shouldBeRunning) { return shouldBeRunning; } + /** Current reader-thread generation. Package-visible for testing. */ + int captureGenerationForTest() { + return captureGeneration; + } + + /** The most recent AudioRecord reader thread. Package-visible for testing. */ + Thread captureThreadForTest() { + return captureThread; + } + /** * Wait for the AudioRecord reader thread to exit before its AudioRecord is * released. Bounded by {@link #CAPTURE_JOIN_TIMEOUT_MS} so a thread stuck diff --git a/ft8af/app/src/main/kotlin/radio/ks3ckc/ft8af/ComposeMainActivity.kt b/ft8af/app/src/main/kotlin/radio/ks3ckc/ft8af/ComposeMainActivity.kt index eca794f61..f5788ea61 100644 --- a/ft8af/app/src/main/kotlin/radio/ks3ckc/ft8af/ComposeMainActivity.kt +++ b/ft8af/app/src/main/kotlin/radio/ks3ckc/ft8af/ComposeMainActivity.kt @@ -404,6 +404,16 @@ class ComposeMainActivity : AppCompatActivity() { "baudRate=${GeneralVariables.baudRate}, " + "controlMode=${GeneralVariables.controlMode}, " + "connectMode=${GeneralVariables.connectMode}") + // Diagnostic (RX device-selection investigation): the value hydration + // actually read from the DB, BEFORE reinitializeAudioInput() below runs + // off it — settles whether a later audioInputDeviceId=0 at MicRecorder + // init means "never persisted / lost on restart" (this line also shows + // 0) or "persisted fine but something reset it afterward" (this line + // shows the real value). + fileLog("configLoaded: audioInputDeviceId=${GeneralVariables.audioInputDeviceId}, " + + "audioOutputDeviceId=${GeneralVariables.audioOutputDeviceId}, " + + "usbAudioInputVidPid=${GeneralVariables.usbAudioInputVendorId}:" + + "${GeneralVariables.usbAudioInputProductId}") if (GeneralVariables.autoUpdateGridFromGPS) { val grid = MaidenheadGrid.getMyMaidenheadGrid(applicationContext) if (grid.isNotEmpty()) { diff --git a/ft8af/app/src/test/java/com/k1af/ft8af/MainViewModelIsQmxRigTest.java b/ft8af/app/src/test/java/com/k1af/ft8af/MainViewModelIsQmxRigTest.java new file mode 100644 index 000000000..e85bdb8e0 --- /dev/null +++ b/ft8af/app/src/test/java/com/k1af/ft8af/MainViewModelIsQmxRigTest.java @@ -0,0 +1,30 @@ +package com.k1af.ft8af; + +import static com.google.common.truth.Truth.assertThat; + +import com.k1af.ft8af.rigs.KenwoodTS590Rig; +import com.k1af.ft8af.rigs.QrpLabsQmxRig; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; + +/** + * {@link MainViewModel#isQmxRig} for the rig-list entries that need a main + * Looper to construct (the Kenwood-family rigs post their first query on it). + * The hamlib cases are in {@link MainViewModelRxTxUsbConflictTest}. + */ +@RunWith(RobolectricTestRunner.class) +public class MainViewModelIsQmxRigTest { + + @Test + public void nativeQmxEntry_isAQmx() { + assertThat(MainViewModel.isQmxRig(new QrpLabsQmxRig())).isTrue(); + } + + @Test + public void itsTs590Parent_isNot() { + // The QDX and other TS-590-protocol rigs share the parent class. + assertThat(MainViewModel.isQmxRig(new KenwoodTS590Rig())).isFalse(); + } +} diff --git a/ft8af/app/src/test/java/com/k1af/ft8af/MainViewModelQmxTxChannelDefaultTest.java b/ft8af/app/src/test/java/com/k1af/ft8af/MainViewModelQmxTxChannelDefaultTest.java new file mode 100644 index 000000000..652b4cd20 --- /dev/null +++ b/ft8af/app/src/test/java/com/k1af/ft8af/MainViewModelQmxTxChannelDefaultTest.java @@ -0,0 +1,47 @@ +package com.k1af.ft8af; + +import static com.google.common.truth.Truth.assertThat; + +import com.k1af.ft8af.wave.AudioChannelSelect; + +import org.junit.Test; + +/** + * Tests {@link MainViewModel#defaultTxChannelForRig} -- the TX audio channel + * used for the rig being connected (real-hardware finding: FT8AF's historical + * {@code BOTH} default underdrives the QMX's modulator; WSJT-X and QFT8 both + * need an explicit mono/left selection for full drive). The QMX gets + * {@code LEFT} only while it is the connected rig and only when the operator + * has never saved a TX channel choice themselves. + */ +public class MainViewModelQmxTxChannelDefaultTest { + + @Test + public void qmxWithNoSavedChoice_usesLeft() { + assertThat(MainViewModel.defaultTxChannelForRig( + true, false, AudioChannelSelect.BOTH)).isEqualTo(AudioChannelSelect.LEFT); + } + + @Test + public void savedChoice_neverOverridden() { + // Even a saved BOTH -- the row's existence means the operator decided. + assertThat(MainViewModel.defaultTxChannelForRig( + true, true, AudioChannelSelect.BOTH)).isEqualTo(AudioChannelSelect.BOTH); + assertThat(MainViewModel.defaultTxChannelForRig( + false, true, AudioChannelSelect.RIGHT)).isEqualTo(AudioChannelSelect.RIGHT); + } + + @Test + public void switchingFromQmxToAnotherRig_goesBackToBoth() { + // The QMX default was applied in memory only (LEFT); the next rig + // connected with no saved choice must not inherit left-only TX. + assertThat(MainViewModel.defaultTxChannelForRig( + false, false, AudioChannelSelect.LEFT)).isEqualTo(AudioChannelSelect.BOTH); + } + + @Test + public void otherRigWithNoSavedChoice_usesBoth() { + assertThat(MainViewModel.defaultTxChannelForRig( + false, false, AudioChannelSelect.BOTH)).isEqualTo(AudioChannelSelect.BOTH); + } +} diff --git a/ft8af/app/src/test/java/com/k1af/ft8af/MainViewModelRxTxUsbConflictTest.java b/ft8af/app/src/test/java/com/k1af/ft8af/MainViewModelRxTxUsbConflictTest.java new file mode 100644 index 000000000..43957fbe8 --- /dev/null +++ b/ft8af/app/src/test/java/com/k1af/ft8af/MainViewModelRxTxUsbConflictTest.java @@ -0,0 +1,115 @@ +package com.k1af.ft8af; + +import static com.google.common.truth.Truth.assertThat; + +import com.k1af.ft8af.rigs.HamlibRig; + +import org.junit.Test; + +/** + * Tests {@link MainViewModel#shouldPauseRxCaptureForTx} -- the decision + * behind pausing RX capture for the duration of one TX "over"/TUNE key-down + * (real-hardware finding: on at least one phone's USB-audio HAL, opening TX + * playback on a USB rig while RX capture from the same USB rig is still open + * fails silently at the ALSA proxy -- confirmed via logcat + * ("pcm_is_ready() failed: cannot set hw params: I/O error", then + * "write(), mPcm is NULL!" on every subsequent write) even though FT8AF's + * generated waveform measured full-scale all the way to the AudioTrack + * write call. A reference app (QFT8) that drives the same radio to full + * output does exactly this: close capture, play, reopen capture). + */ +public class MainViewModelRxTxUsbConflictTest { + + private static final int VID_A = 0x0483; + private static final int PID_A = 0xA34C; + private static final int VID_B = 0x10C4; + private static final int PID_B = 0xEA60; + + @Test + public void micSourceSharingTheUsbDevice_pauses() { + assertThat(MainViewModel.shouldPauseRxCaptureForTx( + /* rigIsQmx= */ true, /* rxIsMicSource= */ true, /* rxIsUsbDirect= */ false, + /* txIsUsbDirectOutput= */ false, + VID_A, PID_A, VID_A, PID_A)).isTrue(); + } + + @Test + public void lanRig_neverPaused() { + // LAN-connected rigs never touch this HAL at all, and HamRecorder's + // doOnWaveDataReceived gates on isRunning -- pausing here would + // spuriously drop their audio too, not just this HAL's. + assertThat(MainViewModel.shouldPauseRxCaptureForTx( + /* rigIsQmx= */ true, /* rxIsMicSource= */ false, /* rxIsUsbDirect= */ false, + /* txIsUsbDirectOutput= */ false, + VID_A, PID_A, VID_A, PID_A)).isFalse(); + } + + @Test + public void rxUsbDirectCapture_neverPaused() { + // Raw-libusb RX capture has no ALSA proxy to conflict with; that path + // is also independently broken for other reasons and shouldn't be + // conflated with this fix. + assertThat(MainViewModel.shouldPauseRxCaptureForTx( + /* rigIsQmx= */ true, /* rxIsMicSource= */ true, /* rxIsUsbDirect= */ true, + /* txIsUsbDirectOutput= */ false, + VID_A, PID_A, VID_A, PID_A)).isFalse(); + } + + @Test + public void txUsbDirectOutput_neverPaused() { + // USB-Direct TX output is a separate, already-broken path + // (LIBUSB_ERROR_IO) -- don't conflate the two bugs. + assertThat(MainViewModel.shouldPauseRxCaptureForTx( + /* rigIsQmx= */ true, /* rxIsMicSource= */ true, /* rxIsUsbDirect= */ false, + /* txIsUsbDirectOutput= */ true, + VID_A, PID_A, VID_A, PID_A)).isFalse(); + } + + @Test + public void differentUsbDevices_neverPaused() { + // Two distinct USB devices (or a USB mic with a non-USB/system + // output) have no reason to share one ALSA card. + assertThat(MainViewModel.shouldPauseRxCaptureForTx( + /* rigIsQmx= */ true, /* rxIsMicSource= */ true, /* rxIsUsbDirect= */ false, + /* txIsUsbDirectOutput= */ false, + VID_A, PID_A, VID_B, PID_B)).isFalse(); + } + + @Test + public void neitherVidPidSet_neverPaused() { + // 0:0 is "no USB device configured" (phone mic / default speaker on + // both sides) -- matching on 0:0 would be a false positive. + assertThat(MainViewModel.shouldPauseRxCaptureForTx( + /* rigIsQmx= */ true, /* rxIsMicSource= */ true, /* rxIsUsbDirect= */ false, + /* txIsUsbDirectOutput= */ false, + 0, 0, 0, 0)).isFalse(); + } + + @Test + public void sameVendorDifferentProduct_neverPaused() { + assertThat(MainViewModel.shouldPauseRxCaptureForTx( + /* rigIsQmx= */ true, /* rxIsMicSource= */ true, /* rxIsUsbDirect= */ false, + /* txIsUsbDirectOutput= */ false, + VID_A, PID_A, VID_A, PID_B)).isFalse(); + } + + @Test + public void nonQmxRigSharingOneUsbCodec_neverPaused() { + // An IC-7300 / IC-705 / Digirig / SignaLink on one USB cable also has + // matching input and output VID:PID. Pausing those would blank the + // waterfall during TX and break full-duplex satellite monitoring for a + // HAL conflict only ever confirmed with the QMX. + assertThat(MainViewModel.shouldPauseRxCaptureForTx( + /* rigIsQmx= */ false, /* rxIsMicSource= */ true, /* rxIsUsbDirect= */ false, + /* txIsUsbDirectOutput= */ false, + VID_A, PID_A, VID_A, PID_A)).isFalse(); + } + + @Test + public void isQmxRig_onlyForTheHamlibQmx() { + assertThat(MainViewModel.isQmxRig(null)).isFalse(); + assertThat(MainViewModel.isQmxRig(new HamlibRig(1036))).isFalse(); // FT-891 + assertThat(MainViewModel.isQmxRig( + new HamlibRig(HamlibRig.QRPLABS_QMX_MODEL_ID))).isTrue(); + } +} diff --git a/ft8af/app/src/test/java/com/k1af/ft8af/database/DatabaseOprHasConfigKeyTest.java b/ft8af/app/src/test/java/com/k1af/ft8af/database/DatabaseOprHasConfigKeyTest.java new file mode 100644 index 000000000..34768404f --- /dev/null +++ b/ft8af/app/src/test/java/com/k1af/ft8af/database/DatabaseOprHasConfigKeyTest.java @@ -0,0 +1,46 @@ +package com.k1af.ft8af.database; + +import static com.google.common.truth.Truth.assertThat; + +import androidx.test.core.app.ApplicationProvider; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; + +import java.util.LinkedHashMap; +import java.util.Map; + +/** + * {@link DatabaseOpr#hasConfigKey} distinguishes "the operator explicitly set + * this" from "this still holds its hardcoded default, nobody's touched it" -- + * a plain read of the hydrated {@code GeneralVariables} field can't tell the + * two apart, since an explicit choice that happens to match the default looks + * identical to never having chosen at all. Used to decide whether it's safe + * to apply a rig-specific default (e.g. the QMX's TX audio channel) without + * overriding a choice the operator already made themselves. + */ +@RunWith(RobolectricTestRunner.class) +public class DatabaseOprHasConfigKeyTest { + + @Test + public void absentKey_returnsFalse() { + DatabaseOpr opr = new DatabaseOpr(ApplicationProvider.getApplicationContext(), null, null, + DatabaseOpr.SCHEMA_VERSION); + assertThat(opr.hasConfigKey("neverWrittenKey")).isFalse(); + } + + @Test + public void writtenKey_returnsTrue_evenIfTheValueMatchesTheDefault() { + DatabaseOpr opr = new DatabaseOpr(ApplicationProvider.getApplicationContext(), null, null, + DatabaseOpr.SCHEMA_VERSION); + Map config = new LinkedHashMap<>(); + // "0" is AudioChannelSelect.BOTH, the same value the field already holds + // before anyone writes anything -- the row's mere existence must still + // count as an explicit choice, not be indistinguishable from absence. + config.put("txAudioChannel", "0"); + opr.writeConfigSync(config); + + assertThat(opr.hasConfigKey("txAudioChannel")).isTrue(); + } +} diff --git a/ft8af/app/src/test/java/com/k1af/ft8af/database/RigAddressAssetTest.java b/ft8af/app/src/test/java/com/k1af/ft8af/database/RigAddressAssetTest.java index 86b384a6a..d1e298bca 100644 --- a/ft8af/app/src/test/java/com/k1af/ft8af/database/RigAddressAssetTest.java +++ b/ft8af/app/src/test/java/com/k1af/ft8af/database/RigAddressAssetTest.java @@ -119,6 +119,7 @@ public class RigAddressAssetTest { "ICOM IC-7300MK2", "YAESU FTX-1(USB)", "QRP Labs QMX", + "Qrp Labs QMX (Hamlib)", }; private RigNameList load() { @@ -205,6 +206,19 @@ public void qmx_isListedWithItsOwnInstructionSetAndQdxBaud() { assertThat(qdx.instructionSet).isEqualTo(InstructionSet.KENWOOD_TS590); } + @Test + public void qmxHamlib_isListedWithHamlibModelId() { + // hamlib's RIG_MODEL_QRPLABS_QMX = RIG_MAKE_MODEL(RIG_KENWOOD, 57) = + // 1000*2+57 = 2057 = 0x809 (see rigs/kenwood/ts480.c's qrplabs_qmx_caps, + // MAX_MODELS_PER_BACKEND=1000 and RIG_KENWOOD=2 in riglist.h). Baud 9600 + // is that backend's declared serial_rate_min. + RigNameList.RigName qmx = findByName(load(), "Qrp Labs QMX (Hamlib)"); + assertThat(qmx).isNotNull(); + assertThat(qmx.address).isEqualTo(0x809); + assertThat(qmx.bauRate).isEqualTo(9600); + assertThat(qmx.instructionSet).isEqualTo(26); // InstructionSet.HAMLIB + } + @Test public void ic7300mk2_addressIsUniqueAndRoundTripsThroughIndexLookup() { RigNameList list = load(); diff --git a/ft8af/app/src/test/java/com/k1af/ft8af/rigs/HamlibRigTest.java b/ft8af/app/src/test/java/com/k1af/ft8af/rigs/HamlibRigTest.java index f57a3a5bf..5981b7e06 100644 --- a/ft8af/app/src/test/java/com/k1af/ft8af/rigs/HamlibRigTest.java +++ b/ft8af/app/src/test/java/com/k1af/ft8af/rigs/HamlibRigTest.java @@ -2,9 +2,19 @@ import static com.google.common.truth.Truth.assertThat; +import com.k1af.ft8af.connector.BaseRigConnector; +import com.k1af.ft8af.database.ControlMode; +import com.k1af.ft8af.database.RigNameList; +import com.k1af.ft8af.wave.HamlibNative; + +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; @@ -108,4 +118,291 @@ public void taskQueuedBeforeDisconnect_doesNotRunBodyAfterClose() throws Excepti // The execution-time re-check dropped B's body after disconnect. assertThat(bBodyRan.get()).isFalse(); } + + /** Captures the last byte[] written, standing in for the real serial link. */ + private static BaseRigConnector recordingConnector(AtomicReference sent) { + return new BaseRigConnector(ControlMode.CAT) { + @Override + public synchronized void sendData(byte[] data) { + sent.set(data); + } + }; + } + + @Test + public void disableQmxIqModeIfApplicable_sendsQ90_forQmxModelId() { + // 0x809 = RIG_MAKE_MODEL(RIG_KENWOOD, 57), hamlib's real QMX model ID + // (see rigaddress.txt's "Qrp Labs QMX (Hamlib),809,...") -- the only + // model this vendor-specific fixup should ever fire for. + HamlibRig rig = new HamlibRig(0x809); + AtomicReference sent = new AtomicReference<>(); + rig.disableQmxIqModeIfApplicable(recordingConnector(sent)); + assertThat(new String(sent.get(), StandardCharsets.US_ASCII)).isEqualTo("Q90;"); + } + + @Test + public void disableQmxIqModeIfApplicable_isNoOp_forOtherModelIds() { + // Only the QMX uses this vendor "Q" command; sending it to an unrelated + // hamlib model (e.g. 1036 = FT-891) would be a meaningless raw write. + HamlibRig rig = new HamlibRig(1036); + AtomicReference sent = new AtomicReference<>(); + rig.disableQmxIqModeIfApplicable(recordingConnector(sent)); + assertThat(sent.get()).isNull(); + } + + // ---- finding 2: FT_MODE must be per-model, not global ---------------- + + @Test + public void ftModeFor_qmx_isRTTY() { + // RTTY -> kenwood_mode_table index 6 -> "MD6;", the QMX's own Digi mode. + assertThat(HamlibRig.ftModeFor(0x809)).isEqualTo("RTTY"); + } + + @Test + public void ftModeFor_otherHamlibRig_isPKTUSB() { + // 1036 = Yaesu FT-891 (Hamlib). PKTUSB (DATA-USB) is correct for it; + // RTTY would wrongly select RTTY-LSB on newcat. + assertThat(HamlibRig.ftModeFor(1036)).isEqualTo("PKTUSB"); + } + + // ---- finding 5: hamlib model id derives from the selected rig entry --- + + @Test + public void modelIdFor_null_isZero() { + assertThat(HamlibRig.modelIdFor(null)).isEqualTo(0); + } + + @Test + public void modelIdFor_nonHamlibEntry_isZero() { + // An ICOM entry's address (0xA4) is a CI-V address, not a hamlib model id. + RigNameList.RigName icom = new RigNameList.RigName( + "ICOM IC-705", 0xA4, 19200, InstructionSet.ICOM); + assertThat(HamlibRig.modelIdFor(icom)).isEqualTo(0); + } + + @Test + public void modelIdFor_qmxEntry_is2057() { + RigNameList.RigName qmx = new RigNameList.RigName( + "Qrp Labs QMX (Hamlib)", 0x809, 9600, InstructionSet.HAMLIB); + assertThat(HamlibRig.modelIdFor(qmx)).isEqualTo(2057); + } + + @Test + public void modelIdFor_ft891Entry_is1036() { + RigNameList.RigName ft891 = new RigNameList.RigName( + "Yaesu FT-891 (Hamlib)", 0x40C, 38400, InstructionSet.HAMLIB); + assertThat(HamlibRig.modelIdFor(ft891)).isEqualTo(1036); + } + + // ---- findings 3/4: open/close races, exercised via a fake NativeApi --- + + /** A connector that reports itself as already connected, with no real transport. */ + private static BaseRigConnector connectedConnector() { + return new BaseRigConnector(ControlMode.CAT) { + @Override + public boolean isConnected() { + return true; + } + }; + } + + /** + * Deterministic fake for {@link HamlibRig.NativeApi}. {@code HamlibNative} + * can't drive these races at all on the JVM ({@code isAvailable()} is + * hard-wired false with no {@code libft8af.so}), so this stands in for the + * native bridge: handles are plain incrementing longs, and {@code + * duringRigOpen} can block the worker thread mid-open to let a test fire a + * concurrent {@code onDisconnecting()} at a precise, repeatable point. + */ + private static class FakeNativeApi implements HamlibRig.NativeApi { + private final AtomicLong nextHandle = new AtomicLong(1); + final List createCalls = new CopyOnWriteArrayList<>(); + final List closeCalls = new CopyOnWriteArrayList<>(); + final List setFreqCalls = new CopyOnWriteArrayList<>(); + volatile int rigOpenRc = 0; + volatile Runnable duringRigOpen; + + @Override public boolean isAvailable() { return true; } + + @Override public long nativeCreate(int modelId, HamlibNative.HamlibSink sink) { + createCalls.add(modelId); + return nextHandle.getAndIncrement(); + } + + @Override public int nativeRigOpen(long handle) { + if (duringRigOpen != null) duringRigOpen.run(); + return rigOpenRc; + } + + @Override public void nativeFeedFromRig(long handle, byte[] data) { } + + @Override public int nativeSetFreq(long handle, long hz) { + setFreqCalls.add(handle); + return 0; + } + + @Override public long nativeGetFreq(long handle) { return -1; } + + @Override public int nativeSetMode(long handle, String mode) { return 0; } + + @Override public int nativeSetPtt(long handle, boolean on) { return 0; } + + @Override public void nativeClose(long handle) { closeCalls.add(handle); } + } + + @Test + public void ensureOpen_failedRigOpen_releasesHandleExactlyOnce() throws Exception { + // Finding 3: nativeRigOpen() no longer frees the bridge itself on failure + // (that used to race nativeFeedFromRig on the serial thread); HamlibRig + // must release it exactly once via releaseHandle()/nativeClose instead. + FakeNativeApi fake = new FakeNativeApi(); + fake.rigOpenRc = -1; // force rig_open() failure + HamlibRig rig = new HamlibRig(2057, fake); + rig.setConnector(connectedConnector()); + + rig.setFreqToRig(); + CountDownLatch drained = new CountDownLatch(1); + rig.submitForTest(drained::countDown); // FIFO marker: runs after setFreqToRig's task + assertThat(drained.await(5, TimeUnit.SECONDS)).isTrue(); + + assertThat(fake.createCalls).containsExactly(2057); + assertThat(fake.closeCalls).containsExactly(1L); + assertThat(fake.setFreqCalls).isEmpty(); // open failed, so the CAT call itself never ran + } + + @Test + public void onDisconnecting_duringInFlightOpen_releasesExactlyOnceWithNoClobber() throws Exception { + // Finding 4: the old onDisconnecting() read/cleared the `handle` field + // directly on the caller's thread, racing the worker thread mid-ensureOpen() + // -- depending on timing that either leaked the bridge ensureOpen was about + // to publish, or let a later ensureOpen() recreate a handle the queued close + // could never release. Now onDisconnecting() only ever submits releaseHandle() + // to the SAME single worker thread ensureOpen() runs on, so the two can never + // interleave -- the queued close always runs strictly after the open finishes. + FakeNativeApi fake = new FakeNativeApi(); + CountDownLatch openStarted = new CountDownLatch(1); + CountDownLatch openMayFinish = new CountDownLatch(1); + fake.duringRigOpen = () -> { + openStarted.countDown(); + try { + openMayFinish.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }; + fake.rigOpenRc = 0; // the open succeeds once released below + + HamlibRig rig = new HamlibRig(2057, fake); + rig.setConnector(connectedConnector()); + + rig.setFreqToRig(); // worker: nativeCreate -> publish handle=1 -> blocks in nativeRigOpen + assertThat(openStarted.await(5, TimeUnit.SECONDS)).isTrue(); + + // "Disconnect" fires here, concurrently with the in-flight open, from this + // (non-worker) thread -- exactly the scenario finding 4 describes. + rig.onDisconnecting(); + + openMayFinish.countDown(); + rig.awaitWorkerForTest(); + + // The open completed using the real handle (1), unclobbered by the + // concurrent disconnect, and the handle was released exactly once. + assertThat(fake.setFreqCalls).containsExactly(1L); + assertThat(fake.closeCalls).containsExactly(1L); + } + + // ---- open-retry backoff: a persistently unresponsive radio ----------- + + @Test + public void shouldAttemptOpen_noPriorFailure_true() { + assertThat(HamlibRig.shouldAttemptOpen(1_000L, Long.MIN_VALUE, 15_000L)).isTrue(); + } + + @Test + public void shouldAttemptOpen_withinBackoffWindow_false() { + assertThat(HamlibRig.shouldAttemptOpen(10_000L, 1_000L, 15_000L)).isFalse(); + } + + @Test + public void shouldAttemptOpen_exactlyAtBackoffBoundary_true() { + assertThat(HamlibRig.shouldAttemptOpen(16_000L, 1_000L, 15_000L)).isTrue(); + } + + @Test + public void shouldAttemptOpen_pastBackoffWindow_true() { + assertThat(HamlibRig.shouldAttemptOpen(20_000L, 1_000L, 15_000L)).isTrue(); + } + + @Test + public void ensureOpen_repeatedCatCallsAfterFailure_doNotRetryWithinBackoff() throws Exception { + // Real-hardware finding: a persistently unresponsive radio (stuck in a + // non-CAT mode) got a brand-new native bridge every single CAT call -- + // 354 of them in ~17 minutes via the app's ~3s CAT-liveness watchdog + // tick. Simulates two such calls landing within the same instant (well + // inside the 15s backoff): only the first should touch the native layer. + // + // Drives a fake clock rather than the real one (the 3-arg constructor): + // android.os.SystemClock.elapsedRealtime() is unmocked and returns 0 in + // this plain (non-Robolectric) JVM test, same as it would for every + // reading in a real run here -- harmless by itself once the "never + // failed" sentinel is Long.MIN_VALUE rather than 0, but a fake clock + // also makes the backoff window itself deterministic instead of + // depending on how fast the test happens to run. + FakeNativeApi fake = new FakeNativeApi(); + fake.rigOpenRc = -1; // every open attempt fails, like an unresponsive radio + AtomicLong clockMs = new AtomicLong(1_000L); + HamlibRig rig = new HamlibRig(2057, fake, clockMs::get); + rig.setConnector(connectedConnector()); + + rig.setFreqToRig(); // first CAT call at t=1000ms: attempts to open, fails + // Drain before moving the clock -- see the sibling retry test's comment; + // this test's own assertion happens to hold either way the race falls, + // but draining first keeps it deterministic rather than accidentally so. + CountDownLatch firstDrained = new CountDownLatch(1); + rig.submitForTest(firstDrained::countDown); + assertThat(firstDrained.await(5, TimeUnit.SECONDS)).isTrue(); + + clockMs.set(1_500L); // 500ms later, well inside the 15s backoff + rig.readFreqFromRig(); // a second CAT call landing moments later (same tick cycle) + CountDownLatch drained = new CountDownLatch(1); + rig.submitForTest(drained::countDown); + assertThat(drained.await(5, TimeUnit.SECONDS)).isTrue(); + + // Only the first call actually touched the native layer; the second was + // skipped by the backoff, not a second failed create/open cycle. + assertThat(fake.createCalls).containsExactly(2057); + assertThat(fake.closeCalls).containsExactly(1L); + } + + @Test + public void ensureOpen_catCallAfterBackoffElapses_retries() throws Exception { + // The other half of the backoff: once the window has genuinely passed, + // the next CAT call must try again, not stay stuck forever. + FakeNativeApi fake = new FakeNativeApi(); + fake.rigOpenRc = -1; + AtomicLong clockMs = new AtomicLong(1_000L); + HamlibRig rig = new HamlibRig(2057, fake, clockMs::get); + rig.setConnector(connectedConnector()); + + rig.setFreqToRig(); // fails at t=1000ms + // Drain the worker before moving the clock: setFreqToRig() only queues the + // task, so without this the clock could jump to 16000 on the test thread + // before the worker has even run the first (t=1000) open attempt, making + // the second call also see "now=16000, lastFailure=16000" (within backoff) + // instead of the intended "lastFailure=1000, now=16000" (past it). + CountDownLatch firstDrained = new CountDownLatch(1); + rig.submitForTest(firstDrained::countDown); + assertThat(firstDrained.await(5, TimeUnit.SECONDS)).isTrue(); + + clockMs.set(1_000L + 15_000L); // exactly at the backoff boundary + rig.readFreqFromRig(); // must retry, not skip + + CountDownLatch drained = new CountDownLatch(1); + rig.submitForTest(drained::countDown); + assertThat(drained.await(5, TimeUnit.SECONDS)).isTrue(); + + assertThat(fake.createCalls).containsExactly(2057, 2057); + // Handles are 1 then 2 -- FakeNativeApi.nextHandle increments per create. + assertThat(fake.closeCalls).containsExactly(1L, 2L); + } } diff --git a/ft8af/app/src/test/java/com/k1af/ft8af/wave/HamRecorderMicGateTest.java b/ft8af/app/src/test/java/com/k1af/ft8af/wave/HamRecorderMicGateTest.java index 6854f8e0c..aeb31baf3 100644 --- a/ft8af/app/src/test/java/com/k1af/ft8af/wave/HamRecorderMicGateTest.java +++ b/ft8af/app/src/test/java/com/k1af/ft8af/wave/HamRecorderMicGateTest.java @@ -32,4 +32,44 @@ public void lanAudioSourceLeavesMicFree() { public void stoppedRecorderLeavesMicFree() { assertThat(HamRecorder.phoneMicCaptureInUse(false, true, false)).isFalse(); } + + /** + * {@link HamRecorder.TxPauseState} -- resumeAfterTx() must only restart + * capture when pauseForTx() actually stopped a running one, never reviving + * capture the operator had deliberately turned off. + */ + @Test + public void txPause_resumesOnlyWhenPauseActuallyStoppedCapture() { + HamRecorder.TxPauseState running = new HamRecorder.TxPauseState(); + assertThat(running.onPause(true)).isTrue(); + assertThat(running.onResume()).isTrue(); + + HamRecorder.TxPauseState stopped = new HamRecorder.TxPauseState(); + assertThat(stopped.onPause(false)).isFalse(); + assertThat(stopped.onResume()).isFalse(); + } + + /** + * A tune carrier and an FT8 over can overlap: the over's key-down can land + * before the tune's key-up. Only the last resume may restart capture, and + * it must still restart it -- the second pause sees capture already + * stopped by the first and must not record "wasn't running". + */ + @Test + public void txPause_nestedPausesRestartOnlyOnTheLastResume() { + HamRecorder.TxPauseState s = new HamRecorder.TxPauseState(); + assertThat(s.onPause(true)).isTrue(); // tune key-down stops capture + assertThat(s.onPause(false)).isFalse(); // over key-down: already paused + assertThat(s.onResume()).isFalse(); // tune key-up: over still keyed + assertThat(s.onResume()).isTrue(); // over key-up restarts capture + } + + @Test + public void txPause_resumeWithoutPauseIsIgnored() { + HamRecorder.TxPauseState s = new HamRecorder.TxPauseState(); + assertThat(s.onResume()).isFalse(); + // and doesn't unbalance a later pause/resume pair + assertThat(s.onPause(true)).isTrue(); + assertThat(s.onResume()).isTrue(); + } } diff --git a/ft8af/app/src/test/java/com/k1af/ft8af/wave/MicRecorderStopRetiresReaderTest.java b/ft8af/app/src/test/java/com/k1af/ft8af/wave/MicRecorderStopRetiresReaderTest.java new file mode 100644 index 000000000..7aa628f51 --- /dev/null +++ b/ft8af/app/src/test/java/com/k1af/ft8af/wave/MicRecorderStopRetiresReaderTest.java @@ -0,0 +1,63 @@ +package com.k1af.ft8af.wave; + +import static com.google.common.truth.Truth.assertThat; + +import com.k1af.ft8af.GeneralVariables; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; + +/** + * Tests that {@link MicRecorder#stopRecord()} retires the AudioRecord reader + * thread before it returns. RX is paused and resumed around every QMX over + * (stopRecord() then start()); if stop only cleared {@code isRunning}, a + * reader still blocked in read() would see {@code isRunning} true again after + * a quick resume and keep reading alongside the new thread -- two readers on + * one AudioRecord is a fatal native abort in libaudioclient. + */ +@RunWith(RobolectricTestRunner.class) +public class MicRecorderStopRetiresReaderTest { + + @Before + public void useThePlainAudioRecordPath() { + GeneralVariables.audioInputDeviceId = 0; + GeneralVariables.usbAudioInputVendorId = 0; + GeneralVariables.usbAudioInputProductId = 0; + } + + @Test + public void stopRecord_supersedesTheReaderGeneration() { + MicRecorder rec = new MicRecorder(); + int before = rec.captureGenerationForTest(); + rec.stopRecord(); + assertThat(rec.captureGenerationForTest()).isGreaterThan(before); + } + + @Test + public void stopRecord_waitsForTheReaderThreadToExit() { + MicRecorder rec = new MicRecorder(); + rec.start(); + Thread reader = rec.captureThreadForTest(); + assertThat(reader).isNotNull(); + rec.stopRecord(); + assertThat(reader.isAlive()).isFalse(); + } + + @Test + public void quickStopThenStart_leavesOnlyTheNewReaderRunning() { + MicRecorder rec = new MicRecorder(); + rec.start(); + Thread first = rec.captureThreadForTest(); + assertThat(first).isNotNull(); + rec.stopRecord(); + rec.start(); + try { + assertThat(first.isAlive()).isFalse(); + assertThat(rec.captureThreadForTest()).isNotSameInstanceAs(first); + } finally { + rec.stopRecord(); + } + } +}