Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions pkgs/bigfred/dcc-bus/cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ type Flags struct {
SingleVehicleControl bool
AllocatePhysicalSlots bool

EnableProgramming bool
DefaultProgrammingTrack string

AllowedOrigins []string
}

Expand Down Expand Up @@ -90,6 +93,10 @@ should rarely be invoked manually.`,
if err != nil {
return fmt.Errorf("dcc-bus station config: %w", err)
}
progTrack, err := ProgrammingTrackFromFlag(f.DefaultProgrammingTrack)
if err != nil {
return fmt.Errorf("dcc-bus programming config: %w", err)
}
cfg := dccbus.Config{
LayoutID: f.LayoutID,
CommandStationID: f.CommandStationID,
Expand Down Expand Up @@ -119,6 +126,8 @@ should rarely be invoked manually.`,
BootStopEnabled: f.BootStopEnabled,
SingleVehicleControl: f.SingleVehicleControl,
AllocatePhysicalSlots: f.AllocatePhysicalSlots,
EnableProgramming: f.EnableProgramming,
DefaultProgrammingTrack: progTrack,
}
d, err := dccbus.New(c.Context(), log, cfg)
if err != nil {
Expand Down Expand Up @@ -161,6 +170,8 @@ should rarely be invoked manually.`,
cmd.Flags().BoolVar(&f.BootStopEnabled, FlagBootStopEnabled, false, "emergency-stop all roster locomotives once after daemon start")
cmd.Flags().BoolVar(&f.SingleVehicleControl, FlagSingleVehicleControl, false, "stop the user's other moving vehicles when driving a different one")
cmd.Flags().BoolVar(&f.AllocatePhysicalSlots, FlagAllocatePhysicalSlots, true, "allocate LocoNet slots like a physical FRED (exclusive IN_USE; disable to piggyback)")
cmd.Flags().BoolVar(&f.EnableProgramming, FlagEnableProgramming, false, "accept decoder CV and address programming frames (loco.cvRead/cvWrite/addrGet/addrSet)")
cmd.Flags().StringVar(&f.DefaultProgrammingTrack, FlagDefaultProgrammingTrack, DefaultProgrammingTrack, "programming track used when a frame omits `mode`: pom (main track) or prog (programming track)")

cmd.AddCommand(newScanCommand(log))

Expand Down
22 changes: 22 additions & 0 deletions pkgs/bigfred/dcc-bus/cli/station.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"strconv"
"strings"

"github.com/keskad/loco/pkgs/bigfred/dcc-bus/protocol"
"github.com/keskad/loco/pkgs/bigfred/server/domain"
)

Expand All @@ -25,8 +26,29 @@ const (
FlagBootStopEnabled = "enable-boot-stop"
FlagSingleVehicleControl = "enable-single-vehicle-control"
FlagAllocatePhysicalSlots = "allocate-physical-slots"
FlagEnableProgramming = "enable-programming"
FlagDefaultProgrammingTrack = "default-programming-track"
)

// DefaultProgrammingTrack is the track CV / address frames land on when
// they omit `mode`. It mirrors domain.DefaultCommandStationProgrammingTrackOutput:
// the isolated programming output cannot disturb locos on the main track.
const DefaultProgrammingTrack = protocol.ProgrammingModeProg

// ProgrammingTrackFromFlag normalises --default-programming-track. An

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✨ [POSITIVE] POSITIVE: The ProgrammingTrackFromFlag function provides clear validation and normalization for the programming track configuration, improving robustness and user experience.

// empty value falls back to DefaultProgrammingTrack.
func ProgrammingTrackFromFlag(track string) (string, error) {
switch t := strings.ToLower(strings.TrimSpace(track)); t {
case "":
return DefaultProgrammingTrack, nil
case protocol.ProgrammingModePOM, protocol.ProgrammingModeProg:
return t, nil
default:
return "", fmt.Errorf("unsupported %s %q (want %q or %q)",
FlagDefaultProgrammingTrack, track, protocol.ProgrammingModePOM, protocol.ProgrammingModeProg)
}
}

// AppendStationFlags appends command-station connection flags for cs.
func AppendStationFlags(args []string, cs domain.CommandStation) []string {
return append(args,
Expand Down
27 changes: 27 additions & 0 deletions pkgs/bigfred/dcc-bus/cli/station_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,33 @@ func TestAppendSingleVehicleControlFlag(t *testing.T) {
}
}

func TestProgrammingTrackFromFlag(t *testing.T) {
for _, tc := range []struct {
in string
want string
wantErr bool
}{
{in: "", want: DefaultProgrammingTrack},
{in: "pom", want: "pom"},
{in: " PROG ", want: "prog"},
{in: "service", wantErr: true},
} {
got, err := ProgrammingTrackFromFlag(tc.in)
if tc.wantErr {
if err == nil {
t.Fatalf("%q: expected an error, got %q", tc.in, got)
}
continue
}
if err != nil {
t.Fatalf("%q: %v", tc.in, err)
}
if got != tc.want {
t.Fatalf("%q = %q, want %q", tc.in, got, tc.want)
}
}
}

func stringsJoin(ss []string) string {
out := ""
for i, s := range ss {
Expand Down
59 changes: 59 additions & 0 deletions pkgs/bigfred/dcc-bus/cmd/control_redis.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"encoding/json"
stderrors "errors"

"github.com/sirupsen/logrus"

"github.com/keskad/loco/pkgs/bigfred/contract"
"github.com/keskad/loco/pkgs/bigfred/dcc-bus/protocol"
"github.com/keskad/loco/pkgs/bigfred/dcc-bus/service"
Expand Down Expand Up @@ -48,6 +50,63 @@ func (r *Router) HandleControlCommand(ctx context.Context, raw []byte) {
return
}
r.applyEStopTarget(ctx, p.Addresses)

case protocol.TypeLocoCVWrite:
var p protocol.LocoCVWritePayload
if err := json.Unmarshal(env.Payload, &p); err != nil {
return
}
r.logControlProgramming(env.Type, r.HandleLocoCVWrite(ctx, controlActor, noopResponder{}, p, ""))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ [RELIABILITY] ControlRedis programming commands bypass programmingEnabled check at the Redis layer.

HandleControlCommand dispatches TypeLocoCVWrite etc. directly to HandleLocoCVWrite, which internally calls programmingGate() — so the rejection still happens. Good. But the control channel is fire-and-forget: logControlProgramming only logs rejections to the daemon log, with no feedback to the server that published the command.

This means a server-side caller (e.g. a future admin "program all decoders" batch) has no way to know its CV write was rejected because programming was disabled on the target daemon. Consider publishing a result frame back on a dcc-bus:cmd-result channel (or at minimum, surfacing CodeProgrammingDisabled in the server's control publisher so it can retry/skip). Not blocking for this PR since the only current caller is the WS proxy path, but worth a TODO.


case protocol.TypeLocoCVRead:
var p protocol.LocoCVReadPayload
if err := json.Unmarshal(env.Payload, &p); err != nil {
return
}
r.logControlProgramming(env.Type, r.HandleLocoCVRead(ctx, controlActor, noopResponder{}, p, ""))

case protocol.TypeLocoAddrSet:
var p protocol.LocoAddrSetPayload
if err := json.Unmarshal(env.Payload, &p); err != nil {
return
}
r.logControlProgramming(env.Type, r.HandleLocoAddrSet(ctx, controlActor, noopResponder{}, p, ""))

case protocol.TypeLocoAddrGet:
var p protocol.LocoAddrGetPayload
if err := json.Unmarshal(env.Payload, &p); err != nil {
return
}
r.logControlProgramming(env.Type, r.HandleLocoAddrGet(ctx, controlActor, noopResponder{}, p, ""))
}
}

// controlActor labels commands that arrive on the Redis control channel
// rather than from a browser session.
var controlActor = Actor{Source: "server"}

// logControlProgramming reports the outcome of a control-channel

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✨ [POSITIVE] POSITIVE: The logControlProgramming function is a good addition for observability. Since Redis control commands are fire-and-forget, logging rejections ensures that operators can diagnose issues with programming commands originating from the control channel.

// programming command. The channel is fire-and-forget, so on rejection
// the daemon publishes a control.programming.rejected event on its
// event channel — that is the server's only signal that the command
// did not run (loco-server can log it, surface it to an admin HUD, or
// retry against a different station). On success the daemon log is
// enough; no event is emitted.
func (r *Router) logControlProgramming(frameType string, res Result) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✨ [POSITIVE] POSITIVE: The logControlProgramming function, which publishes a rejection event to Redis for fire-and-forget commands, is an excellent pattern for providing feedback in an asynchronous system. This ensures that the server has visibility into failed programming attempts.

if res.OK {
return
}
r.log.WithFields(logrus.Fields{
"type": frameType,
"code": res.Code,
}).Warn("dcc-bus control programming command rejected")
if r.redis != nil {
_ = r.redis.Publish(context.Background(), protocol.TypeControlProgrammingRejected,
protocol.ControlProgrammingRejectedPayload{
FrameType: frameType,
Code: res.Code,
Address: res.LocoAddress,
})
}
}

Expand Down
107 changes: 107 additions & 0 deletions pkgs/bigfred/dcc-bus/cmd/control_redis_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package cmd

import (
"context"
"encoding/json"
"io"
"testing"
"time"

"github.com/alicebob/miniredis/v2"
"github.com/redis/go-redis/v9"
"github.com/sirupsen/logrus"

"github.com/keskad/loco/pkgs/bigfred/contract"
buserrors "github.com/keskad/loco/pkgs/bigfred/dcc-bus/errors"
"github.com/keskad/loco/pkgs/bigfred/dcc-bus/protocol"
"github.com/keskad/loco/pkgs/bigfred/dcc-bus/state"
)

func TestLogControlProgramming_publishesRejectionEvent(t *testing.T) {
t.Parallel()
mr, err := miniredis.Run()
if err != nil {
t.Fatalf("miniredis: %v", err)
}
defer mr.Close()

rs := state.NewRedis(redis.NewClient(&redis.Options{Addr: mr.Addr()}), 2, 1)
log := logrus.New()
log.SetOutput(io.Discard)
r := &Router{redis: rs, log: log}

// Subscribe to the daemon's event channel before publishing so the
// message is not lost to fire-and-forget timing.
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
sub := rs.Client().Subscribe(ctx, contract.DccBusEventChannel(2, 1))
defer func() { _ = sub.Close() }()
if _, err := sub.Receive(ctx); err != nil {
t.Fatalf("subscribe: %v", err)
}
msgCh := sub.Channel()

r.logControlProgramming(protocol.TypeLocoCVWrite, Result{
OK: false,
Code: buserrors.CodeProgrammingDisabled,
LocoAddress: 47,
})

select {
case msg := <-msgCh:
var env contract.EnvelopeWire
if err := json.Unmarshal([]byte(msg.Payload), &env); err != nil {
t.Fatalf("unmarshal envelope: %v", err)
}
if env.Type != protocol.TypeControlProgrammingRejected {
t.Fatalf("event type = %q, want %q", env.Type, protocol.TypeControlProgrammingRejected)
}
var p protocol.ControlProgrammingRejectedPayload
if err := json.Unmarshal(env.Payload, &p); err != nil {
t.Fatalf("unmarshal payload: %v", err)
}
if p.FrameType != protocol.TypeLocoCVWrite {
t.Errorf("frameType = %q, want %q", p.FrameType, protocol.TypeLocoCVWrite)
}
if p.Code != buserrors.CodeProgrammingDisabled {
t.Errorf("code = %q, want %q", p.Code, buserrors.CodeProgrammingDisabled)
}
if p.Address != 47 {
t.Errorf("address = %d, want 47", p.Address)
}
case <-time.After(time.Second):
t.Fatal("control.programming.rejected event was not published")
}
}

func TestLogControlProgramming_silentOnSuccess(t *testing.T) {
t.Parallel()
mr, err := miniredis.Run()
if err != nil {
t.Fatalf("miniredis: %v", err)
}
defer mr.Close()

rs := state.NewRedis(redis.NewClient(&redis.Options{Addr: mr.Addr()}), 2, 1)
log := logrus.New()
log.SetOutput(io.Discard)
r := &Router{redis: rs, log: log}

ctx, cancel := context.WithCancel(context.Background())
defer cancel()
sub := rs.Client().Subscribe(ctx, contract.DccBusEventChannel(2, 1))
defer func() { _ = sub.Close() }()
if _, err := sub.Receive(ctx); err != nil {
t.Fatalf("subscribe: %v", err)
}
msgCh := sub.Channel()

r.logControlProgramming(protocol.TypeLocoCVWrite, OKResult())

select {
case msg := <-msgCh:
t.Fatalf("expected no event on success, got %s", msg.Payload)
case <-time.After(150 * time.Millisecond):
// ok — no event published.
}
}
19 changes: 19 additions & 0 deletions pkgs/bigfred/dcc-bus/cmd/port.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,25 @@ type Responder interface {
SendAck(ctx context.Context, requestID string, payload protocol.AckPayload) error
}

// noopResponder satisfies Responder for commands that arrive without a

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✨ [POSITIVE] POSITIVE: The noopResponder is a clean and effective way to satisfy the Responder interface for commands that do not require a client-side response, such as those from the Redis control channel. This avoids unnecessary complexity in the call chain.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✨ [POSITIVE] POSITIVE: The noopResponder is a clean and effective way to satisfy the Responder interface for commands that do not require a direct client response, such as those originating from the Redis control channel.

// client to answer — the Redis control channel is fire-and-forget.
type noopResponder struct{}

func (noopResponder) Subscribe(...uint16) {}
func (noopResponder) Unsubscribe(...uint16) {}
func (noopResponder) SubscribedAddrs() []uint16 { return nil }
func (noopResponder) OldestSubscribed() (uint16, bool) { return 0, false }
func (noopResponder) SelectedAddr() uint16 { return 0 }
func (noopResponder) SetSelected(uint16) {}
func (noopResponder) ClearSelected() {}

func (noopResponder) SendLocoState(context.Context, contract.LocoStateWire) error { return nil }
func (noopResponder) SendLocoError(context.Context, uint16, string, string) error { return nil }
func (noopResponder) SendLocoErrorPayload(context.Context, protocol.LocoErrorPayload) error {
return nil
}
func (noopResponder) SendAck(context.Context, string, protocol.AckPayload) error { return nil }

// SessionView is a snapshot of one live browser session used for fan-out
// and dead-man bookkeeping without importing the ws package.
type SessionView struct {
Expand Down
Loading
Loading