Joystick helper class

Question

I don't use C# very often so would be good to get some feedback on this helper class

using SharpDX.DirectInput;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace SprintTimer
{
    class JoystickHelper
    {
        private DirectInput directInput;
        private Thread pollingThread;
        private Joystick joystick;
        private int startButtonOffset = -1;
        private int lapButtonOffset = -1;


        public JoystickHelper()
        {
            directInput = new DirectInput();
        }

        public List<JoystickDescriptor> DetectDevices() {
            List<JoystickDescriptor> joystickDescriptors = new List<JoystickDescriptor>();

            // check for gamepads
            foreach (var deviceInstance in directInput.GetDevices(DeviceType.Gamepad, DeviceEnumerationFlags.AllDevices))
            {
                joystickDescriptors.Add(new JoystickDescriptor(deviceInstance.InstanceGuid, deviceInstance.InstanceName));
            }

            // check for joysticks
            foreach (var deviceInstance in directInput.GetDevices(DeviceType.Joystick, DeviceEnumerationFlags.AllDevices))
            {
                joystickDescriptors.Add(new JoystickDescriptor(deviceInstance.InstanceGuid, deviceInstance.InstanceName));
            }

            return joystickDescriptors;
        }


        public void StartCapture(Guid joystickGuid, int startButtonOffset, int lapButtonOffset)
        {
            this.startButtonOffset = startButtonOffset;
            this.lapButtonOffset = lapButtonOffset;
            StartCapture(joystickGuid);
        }

        public void StartCapture(Guid joystickGuid)
        {

            joystick = new Joystick(directInput, joystickGuid);

            joystick.Properties.BufferSize = 128;

            joystick.Acquire();

            pollingThread = new Thread(new ThreadStart(PollJoystick));
            pollingThread.Start();

            // Spin for a while waiting for the started thread to become alive
            while (!pollingThread.IsAlive) ;
        }

        public void StopCapture()
        {
            if (pollingThread != null)
            {
                pollingThread.Abort();

                // wait until thread finishes
                pollingThread.Join();
            }

            if (joystick != null)
            {
                joystick.Dispose();
            }
        }

        public void PollJoystick()
        {
            while (true)
            {
                joystick.Poll();
                JoystickUpdate[] datas = joystick.GetBufferedData();
                foreach (JoystickUpdate state in datas)
                {
                    if (state.Offset >= JoystickOffset.Buttons0 && state.Offset <= JoystickOffset.Buttons127)
                    {
                        if (state.Value == 128)
                        {
                            // pressed down
                            JoystickButtonPressedEventArgs args = new JoystickButtonPressedEventArgs();
                            args.ButtonOffset = state.RawOffset;
                            args.TimeStamp = DateTime.Now;

                            OnJoystickButtonPressed(args);

                            if (state.RawOffset == startButtonOffset)
                            {
                                OnJoystickStartButtonPressed(args);
                            }
                            else if (state.RawOffset == lapButtonOffset)
                            {
                                OnJoystickLapButtonPressed(args);
                            }
                        }
                    }

                }

                Thread.Sleep(10);
            }
        }

        protected virtual void OnJoystickButtonPressed(JoystickButtonPressedEventArgs e)
        {
            EventHandler<JoystickButtonPressedEventArgs> handler = JoystickButtonPressed;
            if (handler != null)
            {
                handler(this, e);
            }
        }

        public event EventHandler<JoystickButtonPressedEventArgs> JoystickButtonPressed;

        protected virtual void OnJoystickLapButtonPressed(JoystickButtonPressedEventArgs e)
        {
            EventHandler<JoystickButtonPressedEventArgs> handler = JoystickLapButtonPressed;
            if (handler != null)
            {
                handler(this, e);
            }
        }

        public event EventHandler<JoystickButtonPressedEventArgs> JoystickLapButtonPressed;

        protected virtual void OnJoystickStartButtonPressed(JoystickButtonPressedEventArgs e)
        {
            EventHandler<JoystickButtonPressedEventArgs> handler = JoystickStartButtonPressed;
            if (handler != null)
            {
                handler(this, e);
            }
        }

        public event EventHandler<JoystickButtonPressedEventArgs> JoystickStartButtonPressed;
    }

    public class JoystickButtonPressedEventArgs : EventArgs
    {
        public int ButtonOffset { get; set; }
        public DateTime TimeStamp { get; set; }
    }
}

Usage example

// add a couple of listeners, then initiate capture
joystickHelper.JoystickStartButtonPressed += joystickHelper_JoystickStartButtonPressed;
joystickHelper.JoystickLapButtonPressed += joystickHelper_JoystickLapButtonPressed;
joystickHelper.StartCapture(joystickGuid, joystickStartButtonOffset, joystickLapButtonOffset);

Answer 1

A few things:

1. The DetectDevices method can be shortened with the help of LINQ:

   public IList<JoystickDescriptor> DetectDevices()
   {
       return directInput.GetDevices(DeviceType.Gamepad, DeviceEnumerationFlags.AllDevices)
                         .Concat(directInput.GetDevices(DeviceType.Joystick, DeviceEnumerationFlags.AllDevices))
                         .Select(d => new JoystickDescriptor(d.InstanceGuid, d.InstanceName))
                         .ToList();
   }
   

   I would also question if it is really required to return a List<>. An IEnumerable<> might be sufficient and then you can get rid of the ToList() as well.

2. This is somewhat dubious:

        // Spin for a while waiting for the started thread to become alive
       while (!pollingThread.IsAlive) ;
   

   Not sure what problem this is supposed to solve but I doubt it is really necessary.

3. Finishing a thread by calling Abort() is very nasty. You should introduce a class member flag like _QuitPolling which you set to true once you want to quit. You can then Join() with a timeout and still Abort() if it hasn't finished. Your main loop in PollJoystick would then be changed to

   while (!_QuitPolling)
   {
        ....
   }
   

   And StopCapture would change into:

   public void StopCapture()
   {
       if (pollingThread != null)
       {
           _QuitPolling = true;
           if (!pollingThread.Join(TimeSpan.FromMilliseconds(500)))
           {
               pollingThread.Abort();
           }
       }
   
       if (joystick != null)
       {
           joystick.Dispose();
       }
   }
   

4. The multiple of data is still data and not datas.

5. The main thread method PollJoystick can be simplified by introducing a little helper method which filters out relevant updates and again a bit of LINQ:

   Helper method:

   private bool IsRelevantUpdate(JoystickUpdate state)
   {
       return state.Offset >= JoystickOffset.Buttons0 && state.Offset <= JoystickOffset.Buttons127 && state.Value == 128;
   }
   

   Refactored main loop:

   JoystickUpdate[] data = joystick.GetBufferedData();
   foreach (JoystickUpdate state in data.Where(IsRelevantUpdate))
   {
       ...
   }
   

   Resulting in nesting reduced by 2 levels.

---

Update:

DirectInput as well as Joystick are IDisposable. The general rules around this are:

1. If you create an object which is IDisposable and you own it (your code determines the lifetime and ownership is not transferred to another entity) then you are required to dispose of it after you are finished using it.
2. If the IDisposable object you own is a class member then your class should become IDisposable as well disposing of any members in its own Dispose method.

If these rules are followed you are less likely to forget to dispose of objects which require it.

For your code this means that you should implement IDisposable which could just call StopCapture. In addition to joystick you should also dispose of directInput at that point (which means that you probably should move the creation of directInput into StartCapture if you want to start/stop multiple times).

Answer 2

To insure backward compatibility, Microsoft decided to allow you to use Threads in your code. However, I blame your IDE for not displaying any massive warnings when you wrote this code because as you said, you aren't an experienced C# programmer. The reason why I saying that is because YOU SHOULD NEVER use Threads in your code. Threading is hard for people like me and you that aren't experts in multi-threading.If you wanna do me a favour, go to your editor and do find: Thread and delete all its occurrences, it's only acceptable to see the term Thread in your code if you doing Thread.Sleep(0), otherwise it's wrong. So what we do, we use Tasks.

In your code you can easily replace pollingThread by a pollingTask

private Task pollingTask;

And to start the task

 public void PollJoystick()
 {
     pollingTask = Task.Factory.StartNew(()=>{
         while(true){
            .....
         }

     });
 }

Now you want to wait until the task is done, it's fine

public void StopCapture()
    {
        if (pollingTask != null)
        {
            pollingTask.Wait(); // you can give this Wait a timeout
        }

    }

If you want to Sleep never use Thread.Sleep(1000), this is quite expensive and unnecessary, use Task.Delay(1000) instead. You really need to rewrite your code and use the TPL library instead of Threads. Give it a go, and post your code again to be reviewed but with Tasks this time